From 503aa28c4794130d8e40f53c1e9dc7f9fe3bab08 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Fri, 22 Mar 2024 13:54:35 -0500 Subject: [PATCH] DPoP nonce updates from review discussion --- .../ConfigureOpenIdConnectOptions.cs | 4 +- .../DPoPProofTokenHandler.cs | 61 +++++++++++-------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/src/Duende.AccessTokenManagement.OpenIdConnect/ConfigureOpenIdConnectOptions.cs b/src/Duende.AccessTokenManagement.OpenIdConnect/ConfigureOpenIdConnectOptions.cs index 15c7e01..b93aa79 100644 --- a/src/Duende.AccessTokenManagement.OpenIdConnect/ConfigureOpenIdConnectOptions.cs +++ b/src/Duende.AccessTokenManagement.OpenIdConnect/ConfigureOpenIdConnectOptions.cs @@ -78,9 +78,7 @@ public void Configure(string? name, OpenIdConnectOptions options) options.Events.OnAuthorizationCodeReceived = CreateCallback(options.Events.OnAuthorizationCodeReceived); options.Events.OnTokenValidated = CreateCallback(options.Events.OnTokenValidated); - var logger = _loggerFactory.CreateLogger(); - - options.BackchannelHttpHandler = new DPoPProofTokenHandler(_dPoPProofService, _dPoPNonceStore, _httpContextAccessor, logger) + options.BackchannelHttpHandler = new AuthorizationServerDPoPHandler(_dPoPProofService, _dPoPNonceStore, _httpContextAccessor, _loggerFactory) { InnerHandler = options.BackchannelHttpHandler ?? new HttpClientHandler() }; diff --git a/src/Duende.AccessTokenManagement.OpenIdConnect/DPoPProofTokenHandler.cs b/src/Duende.AccessTokenManagement.OpenIdConnect/DPoPProofTokenHandler.cs index ae5adf9..20aab2f 100644 --- a/src/Duende.AccessTokenManagement.OpenIdConnect/DPoPProofTokenHandler.cs +++ b/src/Duende.AccessTokenManagement.OpenIdConnect/DPoPProofTokenHandler.cs @@ -1,9 +1,10 @@ -// Copyright (c) Brock Allen & Dominick Baier. All rights reserved. +// Copyright (c) Brock Allen & Dominick Baier. All rights reserved. // Licensed under the Apache License, Version 2.0. See LICENSE in the project root for license information. using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Logging; +using System.Net; using System.Net.Http; using System.Threading; using System.Threading.Tasks; @@ -11,26 +12,30 @@ namespace Duende.AccessTokenManagement.OpenIdConnect; /// -/// Delegating handler that injects the DPoP proof token. This is intended to be -/// used on the OIDC authentication handler's backchannel http client. +/// Delegating handler that injects the DPoP proof token into requests that are +/// made to the authorization server. This is intended to be used on the OIDC +/// authentication handler's backchannel http client. /// -public class DPoPProofTokenHandler : DelegatingHandler +internal class AuthorizationServerDPoPHandler : DelegatingHandler { private readonly IDPoPProofService _dPoPProofService; private readonly IDPoPNonceStore _dPoPNonceStore; private readonly IHttpContextAccessor _httpContextAccessor; - private readonly ILogger _logger; + private readonly ILogger _logger; - internal DPoPProofTokenHandler( + internal AuthorizationServerDPoPHandler( IDPoPProofService dPoPProofService, IDPoPNonceStore dPoPNonceStore, IHttpContextAccessor httpContextAccessor, - ILogger logger) + ILoggerFactory loggerFactory) { _dPoPProofService = dPoPProofService; _dPoPNonceStore = dPoPNonceStore; _httpContextAccessor = httpContextAccessor; - _logger = logger; + // We depend on the logger factory, rather than the logger itself, since + // the type parameter of the logger (referencing this class) will not + // always be accessible. + _logger = loggerFactory.CreateLogger(); } /// @@ -39,32 +44,38 @@ protected override async Task SendAsync(HttpRequestMessage await SetDPoPProofTokenAsync(request, cancellationToken).ConfigureAwait(false); var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); - // The authorization server might send us a new nonce in a successful - // request, indicating that we should use the new nonce in future. + // The authorization server might send us a new nonce on either a success or failure var dPoPNonce = response.GetDPoPNonce(); if (dPoPNonce != null) { - _logger.LogDebug("The authorization server has supplied a new nonce"); - - await _dPoPNonceStore.StoreNonceAsync(new DPoPNonceContext - { - Url = request.GetDPoPUrl(), - Method = request.Method.ToString(), - }, dPoPNonce); - - // But the authorization server might also send a failure response, and expect us to retry - if (response.StatusCode == System.Net.HttpStatusCode.BadRequest) + // If the response code is a bad request, we can infer that we + // should retry with the new nonce. The server should have also set + // the error: use_dpop_nonce, but there's no need to incur the cost + // of parsing the json and checking for that, as we would only + // receive the nonce http header when that error was set. + // Authorization servers might preemptively send a new nonce, but + // the spec specifically says to do that on a success (and we handle + // that case in the else block) + // + // TL;DR - presence of nonce and 400 response code is enough to + // trigger a retry + if (response.StatusCode == HttpStatusCode.BadRequest) { - - // REVIEW: Is it good enough to check the status code and - // existence of the new nonce? Should we parse the response, and - // look for the "use_dpop_nonce" value in the error property? - _logger.LogDebug("Request failed (bad request). Retrying request with new DPoP proof token that includes the new nonce"); response.Dispose(); await SetDPoPProofTokenAsync(request, cancellationToken, dPoPNonce).ConfigureAwait(false); return await base.SendAsync(request, cancellationToken).ConfigureAwait(false); + } + else if (response.StatusCode == HttpStatusCode.OK) + { + _logger.LogDebug("The authorization server has supplied a new nonce on a successful response, which will be stored and used in future requests to the authorization server"); + + await _dPoPNonceStore.StoreNonceAsync(new DPoPNonceContext + { + Url = request.GetDPoPUrl(), + Method = request.Method.ToString(), + }, dPoPNonce); } }