Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
329 changes: 329 additions & 0 deletions PR_SUMMARY.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,329 @@
# Certificate API Consolidation - Summary for PR Review

## Quick Overview

This PR consolidates 7+ fragmented certificate APIs into a clean, user-friendly, and future-proof design for MSAL.NET confidential client applications.

## The Problem (Before)

```csharp
// Too many methods, confusing booleans, scattered options
builder.WithCertificate(cert, sendX5C: true)
builder.WithClientClaims(cert, claims, merge: true, sendX5C: true)
builder.WithCertificate(cert, sendX5C: true, associateTokensWithCertificateSerialNumber: true)

// mTLS required at request level
app.AcquireTokenForClient(scopes)
.WithMtlsProofOfPossession()
.ExecuteAsync()
```

**Issues:**
- 7+ different methods across multiple classes
- Boolean soup (`sendX5C`, `merge`, etc.)
- Hard to discover features
- Can't choose bearer vs PoP with mTLS
- No built-in claims challenge support

## The Solution (After)

### Current Implementation: CertificateConfiguration

```csharp
var certConfig = new CertificateConfiguration(certificate)
{
SendX5C = true,
EnableMtlsProofOfPossession = true,
UseBearerTokenWithMtls = false, // PoP (default) or Bearer
Claims = claimsChallenge
};

var app = builder
.WithCertificateConfiguration(certConfig)
.WithAzureRegion("eastus")
.Build();

// mTLS and claims auto-applied
var result = await app.AcquireTokenForClient(scopes).ExecuteAsync();
```

### Proposed Fluent API (In Spec for Future)

```csharp
var app = builder
.WithCertificate(certificate)
.SendCertificateChain()
.UseMutualTls()
.WithProofOfPossession() // or .WithBearerToken()
.And()
.PartitionCacheBySerialNumber()
.WithAzureRegion("eastus")
.Build();

var result = await app.AcquireTokenForClient(scopes).ExecuteAsync();
```

## What This PR Includes

### 1. New CertificateConfiguration Class

All certificate options in one place:

```csharp
public sealed class CertificateConfiguration
{
public X509Certificate2 Certificate { get; } // Required
public bool SendX5C { get; set; } // X5C chain
public bool EnableMtlsProofOfPossession { get; set; } // Enable mTLS
public bool UseBearerTokenWithMtls { get; set; } // PoP or Bearer
public IDictionary<string, string> ClaimsToSign { get; set; } // JWT claims
public bool MergeWithDefaultClaims { get; set; } // Merge option
public bool AssociateTokensWithCertificateSerialNumber { get; set; } // Cache partition
public string Claims { get; set; } // Conditional Access
}
```

### 2. WithCertificateConfiguration Method

```csharp
public ConfidentialClientApplicationBuilder WithCertificateConfiguration(
CertificateConfiguration certificateConfiguration);
```

### 3. Bearer Token Support Over mTLS

New `MtlsBearerAuthenticationOperation` class for bearer tokens with mTLS transport.

### 4. Auto-Apply at Request Time

Configuration automatically applied in `AcquireTokenForClient`, no need for request-level methods.

### 5. Comprehensive Documentation

- API specification (26K words)
- User guide with examples
- Migration guide from old APIs
- Alignment with PR #5399 and issue #5568

### 6. Full Test Coverage

15+ test methods covering all scenarios.

## Key Benefits

| Before | After |
|--------|-------|
| 7+ methods | 1 method |
| Boolean parameters | Named properties |
| Request-level mTLS | Builder-level config |
| No bearer option | PoP or Bearer choice |
| Manual claims | Auto-applied claims |

## Real-World Examples

### Example 1: Basic Certificate

```csharp
var app = builder
.WithCertificateConfiguration(new CertificateConfiguration(cert))
.Build();
```

### Example 2: mTLS with PoP (Most Secure)

```csharp
var app = builder
.WithCertificateConfiguration(new CertificateConfiguration(cert)
{
EnableMtlsProofOfPossession = true
})
.WithAzureRegion("eastus")
.Build();
```

### Example 3: mTLS with Bearer

```csharp
var app = builder
.WithCertificateConfiguration(new CertificateConfiguration(cert)
{
EnableMtlsProofOfPossession = true,
UseBearerTokenWithMtls = true
})
.WithAzureRegion("eastus")
.Build();
```

### Example 4: Everything Together

```csharp
var app = builder
.WithCertificateConfiguration(new CertificateConfiguration(cert)
{
SendX5C = true,
EnableMtlsProofOfPossession = true,
UseBearerTokenWithMtls = false,
ClaimsToSign = customClaims,
AssociateTokensWithCertificateSerialNumber = true,
Claims = claimsChallenge
})
.WithAzureRegion("eastus")
.Build();
```

## Migration Path

All existing APIs continue to work! Migration is optional.

```csharp
// OLD - Still works!
builder.WithCertificate(cert, sendX5C: true)

// NEW - Recommended
builder.WithCertificateConfiguration(new CertificateConfiguration(cert)
{
SendX5C = true
})
```

## Alignment with Team Vision

### PR #5399 (Bound Client Assertion)

**PR #5399 proposes:** `AssertionResponse` with optional certificate binding

**This PR provides:** Certificate-first approach that complements assertion scenarios

**Together they support:**
- Certificate scenarios → Use `CertificateConfiguration`
- Assertion scenarios → Use `WithClientAssertion` with `AssertionResponse`

### Issue #5568 (API Consolidation)

**Goals from #5568:**
- ✅ Consolidate fragmented APIs
- ✅ Support bearer and PoP with mTLS
- ✅ Built-in claims challenge support
- ✅ Future-proof extensibility

**All addressed in this PR!**

## Future-Proofing

Easy to add new features without breaking changes:

```csharp
// Future additions (examples)
public class CertificateConfiguration
{
// Existing properties...

// Future:
public TimeSpan? CertificateRefreshInterval { get; set; }
public ICertificateRotationStrategy RotationStrategy { get; set; }
public string FmiPath { get; set; }
}
```

## The Fluent API Alternative

The specification also proposes a fluent API for future consideration:

```csharp
builder
.WithCertificate(cert)
.SendCertificateChain()
.UseMutualTls()
.WithProofOfPossession()
```

**Pros:**
- More discoverable via IntelliSense
- Reads like natural language
- Prevents invalid combinations

**Cons:**
- More implementation work
- Slightly more complex

**Recommendation:** Start with `CertificateConfiguration`, evaluate fluent API based on feedback.

## Files to Review

### Code Changes
1. **`CertificateConfiguration.cs`** - New configuration class
2. **`ConfidentialClientApplicationBuilder.cs`** - New `WithCertificateConfiguration` method
3. **`ApplicationConfiguration.cs`** - Added properties for config storage
4. **`AcquireTokenForClientParameterBuilder.cs`** - Auto-apply logic
5. **`MtlsBearerAuthenticationOperation.cs`** - Bearer over mTLS support

### Tests
6. **`CertificateConfigurationTests.cs`** - 15+ comprehensive tests

### Documentation
7. **`certificate_configuration_consolidation.md`** - User guide
8. **`alignment_with_pr5399_and_issue5568.md`** - Design rationale
9. **`certificate_api_consolidation_spec.md`** - **Complete specification** ⭐

### Public API
10. **`PublicAPI.Unshipped.txt`** (all platforms) - API analyzer updates

## Questions for Reviewers

1. **API Design:** `CertificateConfiguration` vs Fluent Builder?
2. **Naming:** Are property names clear and intuitive?
3. **Defaults:** Should mTLS default to PoP (current) or Bearer?
4. **Deprecation:** When (if ever) should old APIs be marked obsolete?
5. **Missing Features:** Anything else we should include?

## Testing Done

- ✅ All existing tests pass
- ✅ 15+ new tests for new features
- ✅ Build succeeds with no warnings
- ✅ PublicAPI analyzer happy
- ✅ Manual testing of all scenarios

## Backward Compatibility

- ✅ All existing APIs work unchanged
- ✅ No breaking changes
- ✅ Existing code continues to compile
- ✅ No behavior changes for existing code

## Performance Impact

- ✅ No runtime performance impact
- ✅ Configuration happens at build time
- ✅ Same token acquisition performance
- ✅ No additional memory allocations

## Next Steps

1. Review specification document
2. Discuss API design choice (config object vs fluent)
3. Gather feedback on property names
4. Decide on default behaviors
5. Plan documentation updates
6. Consider preview release for community feedback

## Summary

This PR consolidates fragmented certificate APIs into a clean, maintainable design that:

- ✅ Simplifies certificate configuration
- ✅ Supports all current scenarios
- ✅ Adds new features (bearer with mTLS, claims)
- ✅ Maintains backward compatibility
- ✅ Enables future enhancements
- ✅ Aligns with team's long-term vision
- ✅ Improves developer experience

**Ready for review!** 🚀

---

**Key Documents:**
- 📄 Full Specification: `docs/specs/certificate_api_consolidation_spec.md`
- 📖 User Guide: `docs/certificate_configuration_consolidation.md`
- 🔗 Alignment Doc: `docs/alignment_with_pr5399_and_issue5568.md`
Loading