test(backends): cover the paths an operator's actions reach (superseded by #291, tests landed on develop) - #276
Closed
kirill-abblix wants to merge 12 commits into
Closed
test(backends): cover the paths an operator's actions reach (superseded by #291, tests landed on develop)#276kirill-abblix wants to merge 12 commits into
kirill-abblix wants to merge 12 commits into
Conversation
Add the framework-agnostic client core project under src/ and its unit test project under tests/, wired into the solution and the PR test matrix. The core exposes AddOidcClientCore, which binds OidcClientOptions; feature services and the request and validation pipelines land in the following commits. Mirrors the Abblix.Jwt project conventions: multi-target, package metadata, InternalsVisibleTo.
Align the newly scaffolded Abblix.Oidc.Client.UnitTests with the test-stack upgrade merged from develop: it referenced the xunit.v3 metapackage, which no longer carries a central version after the switch to the mtp-v2 variant.
Adds the first feature of the client core: where the provider's endpoints come from. Every later feature reads them through IProviderMetadataProvider, so a provider that moves an endpoint is followed automatically. Two sources answer that contract. DiscoveredMetadataProvider reads the document the provider publishes, verifies the issuer it declares against the authority it was fetched from (OpenID Connect Discovery 1.0 section 4.3) and caches it. ConfiguredMetadataProvider serves endpoints the host wrote by hand, for the many OAuth 2.0 providers that publish no document at all. The host names its source: AddDiscovery or AddConfiguredMetadata. Nothing is inferred from the shape of the configuration, and there is no default - reading endpoints from a signed-off document and taking them on the host's word are different trust models. Omitting the choice leaves a guard registered that fails with a message naming both calls, mirroring the external-key tier guard. Discovery settings live in their own DiscoveryOptions rather than on the client options, so a client that is told its endpoints never sees settings it cannot use. Unmodelled members of the document are kept rather than discarded, so a paid layer or a host can read a provider capability the base client has no opinion about. 15 unit tests, solution builds clean.
Adds the source of the keys that verify the provider's signatures, read from the address its metadata names and parsed with Abblix.Jwt. Rotation is handled here rather than by callers. A provider replaces its keys on its own schedule, so a token naming a key the client has not seen is a normal event: it triggers a re-read of the key set instead of a rejection. That path is driven by whoever presents the token, so it has a floor - without one, a stream of tokens naming random keys turns this client into a load generator against its own provider. A key still unknown after the re-read leaves every held key on the table, because deciding "no such key" here would turn a labelling difference into a silent authentication failure. Keys marked for encryption are not offered for signature verification, which is what the `use` member exists for. The floor and the cache are per client instance, so N replicas allow N reads per window rather than one. Said plainly in the options documentation, and the contract is registered with TryAdd so an application that needs a bound across replicas supplies its own implementation. Making a free base client depend on shared storage to get that bound would be the wrong trade. Also extracts the fetch-and-hold behaviour both this and the discovery document wanted into one internal primitive, and fixes a real defect found while testing it: concurrent forced refreshes were deduplicated by comparing timestamps, which a clock too coarse to separate two adjacent operations resolves in favour of the entry the refresh was asked to replace. Now compared by entry identity. Cache lifetimes now carry downward jitter, so replicas started by one rollout stop expiring in lockstep and going to the provider in a single wave. 24 unit tests, solution builds clean.
The cache serialised callers with a semaphore, which gives mutual exclusion but not a shared result. Two consequences, both wrong in the direction that matters: callers woke one at a time only to find the value already there, and during an outage each waiter went on to make its own failing attempt, so a burst of requests became a queue of round trips against a provider already in trouble. Callers now share the attempt itself. One publishes it, everyone who arrives while it is in flight awaits the same one, and a failure is shared rather than repeated. The attempt runs without any single caller's cancellation token, so one caller giving up no longer cancels the read everyone else is waiting on; each abandons only its own wait. Also adds a fixed set of verification keys as an alternative to reading the provider's key set: for a provider that publishes none, a deployment that cannot reach the one it publishes, or an operator pinning keys deliberately. The trade is that a rotation then needs a reconfiguration, so it is a choice rather than a default, registered the same way the metadata source is. 29 unit tests, five of them on the cache itself, including concurrent sharing of both a successful and a failing read.
Adds the outbound half of signing in: the address the user is sent to, and everything the client must put aside in order to judge their return. The three arrive together because they are one concern. An authorization response comes back on a separate request, from the browser, carrying only what the provider chose to echo, so every value used to judge it has to be stored beforehand: the state that says which request it answers, the nonce that ties the id_token to it, the code verifier that proves the exchange, the issuer that was actually asked, and where the user was heading. PKCE is SHA-256 and nothing else. RFC 7636 also defines `plain`, where the challenge is the verifier itself, and falling back to it when a provider looks unable to do better is the downgrade: anyone who reads the request then holds the verifier while the request still looks protected. A provider advertising only `plain` therefore gets a refusal rather than a weaker request. One that advertises nothing is given the benefit of the doubt, since the member is optional and a request the provider cannot honour fails at the provider. Stored state is single-use: taking it removes it in the same step. Left in place, a captured authorization response could be replayed and every replay would find its state waiting and look entirely legitimate. Entries also expire and are swept on write, because they carry a code verifier and a store that only grows is a slow leak driven by anyone able to start a sign-in. The in-memory store is correct for one instance only, and says so: a callback may land on any replica. The cookie-backed store of the ASP.NET adapter is the answer there, and it travels with the user rather than living on one node. Resource indicators are sent one parameter per resource rather than collapsed, since RFC 8707 lets it repeat and keeping only the last would silently widen what the token is good for. That needed an append to the shared parameters builder, which until now could only replace. Names follow the server side of the family: the feature is Pkce, as in PkceValidator, and the challenge methods carry the same constant names. The earlier ProofKeys naming was wrong twice over, since it also reached into the vocabulary of DPoP, which is a different feature. 44 unit tests, solution builds clean.
Adds the exchange of an authorization code for tokens and the refresh of an expired set, with the client authentication both need. The client authentication method is configured, not inferred from what the provider advertises. Picking the strongest method a provider claims to support sounds accommodating, but it hands the provider's own document the decision of how this client proves who it is, and a downgrade arrived at that way is silent. A public client is the default, since a client with no secret cannot leak one; a confidential client says so and supplies its secret. Basic credentials form-encode both halves before joining them, as RFC 6749 section 2.3.1 requires. Without it a secret containing a colon or a space reaches the provider as a different secret, and the failure reads as a wrong password rather than a wrong encoding. The error code a refusal carries is kept on the exception rather than folded into its message, because callers act on it: a refresh answered with invalid_grant means the presented token has been rotated away, which is recoverable, and every other code is not. A refusal whose body does not follow the documented shape is still a refusal, so an unreadable body cannot mask the status. The token endpoint gets its own named HttpClient. That is the seam a paid layer needs in order to hang certificate-bound mutual TLS or the DPoP nonce retry on this exact traffic, without either reaching into request building. Unmodelled members of the response are kept, as elsewhere. Single-flight refresh per session is deliberately NOT here. It sits above this service, needs the session store to exist first, and is recorded in the design with the two tests it must pass. 55 unit tests, solution builds clean.
Measured line coverage of Abblix.Oidc.Client at 87.0% and found one real hole rather than a diffuse shortfall: ConfiguredSigningKeysProvider had no test at all. A source of verification keys with zero tests is the kind of thing that looks present and is not, so it is covered first: key selection, the fallback when the named key was never configured, and the refusal of an empty set. Also covers what the measurement showed dark in the token service - the transport failure, a success whose body cannot be read, a success carrying a literal null, and an authentication method this client cannot present - plus a first test of the registration extensions themselves. The features were tested by constructing services directly, which says nothing about whether the extensions name the right lifetimes and dependencies; a missing registration surfaces only when something asks the container, so now something does, with ValidateOnBuild and ValidateScopes on. 87.0% to 98.6% of lines, 88.6% of branches, 65 tests.
Branch coverage in the two custodian packages sat far below the rest of the solution, and reading the source rather than the number showed why: the happy path was exercised and the answers we do not like were not. These are clients whose whole purpose is keeping a private key inside an HSM, so what they do when the custodian refuses is the interesting half. Most of what looked missing turned out to be covered already - a rejected ciphertext becoming null, a 403 throwing, an unsupported algorithm refused before any call. What was genuinely dark: A disabled Key Vault version was never tested. Disabling a version is how an operator takes a compromised key out of service; had the skip been wrong, the key would have stayed in the JWKS and stayed eligible to sign, and the revocation would have appeared to succeed while changing nothing. A version with no creation time was never tested. The creation time decides which version signs and when a rotation takes over, so a version whose age is unknown has to stop the enumeration rather than be dated to year one. Vault threw only on 403. A 429, a 503 and a 500 are not decryption failures either, and reporting one as null would reject every encrypted token for as long as the fault lasts while blaming the clients for a good JWE. Transit will hold an ed25519 or symmetric key and an operator can point this store at one by configuring a key name. The refusal has to name the type, or the failure surfaces as an import error deep in the crypto stack, far from the configuration that caused it. Vault's signing algorithm table is now exercised arm by arm against the wire. A hand-written mapping invites transposition, and a transposed arm is not a crash: Vault signs with whatever it is told, so the token goes out under one algorithm while its header advertises another. Each new test was verified by mutation - the disabled-version skip and the unwrap status filter were each broken in the production source and the tests went red, then the source was restored. A test that cannot fail is not protecting anything. Azure's mapping table could not be asserted the same way: the SDK's transport hands the message handler a request whose content is already gone, so the algorithm actually sent is not observable through this seam. Recorded on the issue rather than worked around. Vault branches 51.9% to 76.4%, lines 91.9% to 98.5%. Azure lines 78.5% to 84.2%. No production code changed.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+66
to
+69
| var response = new HttpResponseMessage(_statusCode) | ||
| { | ||
| Content = new StringContent(_body), | ||
| }; |
Two properties the suite asserted nowhere, both reachable only end to end. Revocation was exercised only as a side effect of reuse detection, which revokes a family on the server's own initiative. The endpoint a client actually calls was never walked, so nothing checked the property that matters: whether a revoked token stops working. An endpoint that answers 200 and leaves the token usable is worse than none, because it reports that the danger has been dealt with. Now covered along with the two rules that keep it from becoming a weapon: an unknown token still answers 200, so the endpoint cannot be used to test whether a guessed token exists, and an unauthenticated request is rejected without revoking, so it cannot be used to knock out another client's tokens. Discovery must omit what the provider does not have rather than publish it as null. The two are not interchangeable to a reader: a client decides what a provider supports by asking whether a member is present, so a null reads as a capability that exists and then fails at the moment it is used. The guarantee does not live in the model but in one serializer modifier per adapter, which a refactoring of how those options are built would drop with no compiler complaint, so it is asserted on the wire and across both well-known paths. Both were verified by mutation. Removing the modifier makes the discovery test name exactly what would have leaked: mtls_endpoint_aliases, acr_values_supported, signed_metadata. That mutation also corrected a wrong assumption of mine worth recording: the modifier attached in ConfigurationResponseFormatter serves the signed_metadata JWS payload, not the plain document, whose omission is configured in AddOidcControllers. The signed copy's own nulls are therefore still unasserted - RFC 8414 gives signed values precedence, so a null there would override the plain document. It needs a host with signed metadata enabled and is left for the issue. No production code changed. 91 E2E tests green.
AddMtlsCertificateForwarding was the largest untested file in the adapter and the one with the most at stake. Its header converter turns a value written by a reverse proxy into the X.509 certificate that mTLS client authentication then treats as the client's identity, so it decides who the caller is. Two failure modes, neither cosmetic. Producing a certificate other than the one the header carried authenticates the wrong client, so every parse test compares thumbprints rather than merely asserting that something parsed. Failing to read a shape a real proxy emits locks a correctly configured client out, so the three shapes in the wild are each covered - PEM from nginx with ssl_client_escaped_cert, percent-encoded base64 from nginx with ssl_client_cert, and raw base64 from Envoy and HAProxy - along with the two ways a value arrives mangled in practice, padding dropped in transit and lines wrapped. The rejection side matters as much: the header comes from outside, so junk must produce nothing at all rather than an identity nobody presented. Exercised through the options the extension registers, not as a private function. What the middleware calls is what is registered, and testing a copy would prove nothing about it. Verified by mutation: removing the base64 padding normalisation reddens exactly one test, the unpadded one. No production code changed. 88 Mvc unit tests green.
|
Member
Author
kirill-abblix
changed the title
Member
Author
|
Closed without merging, but none of the work here was lost. Recording where it went, because the branch has now been deleted and the commit ids will not resolve. Of the twelve commits that were not ancestors of
The reason this needed checking at all: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the custodian half of #275.
What was actually wrong
Branch coverage in the two custodian packages sat far below the rest of the solution (Vault 51.9%, Azure 45.9%, everything else 71-89%). Reading the source rather than the number gave the reason: the happy path was exercised and the answers we do not like were not.
Most of what looked missing was already covered - a rejected ciphertext becoming
null, a 403 throwing, an unsupported algorithm refused before any call. The issue's framing was a hypothesis and the source corrected it. What was genuinely dark:nullwould reject every encrypted token for as long as the fault lasts, silently, and blame the clients for a JWE that is perfectly good.Verification
Every new test was checked by mutation, not by watching it go green. The disabled-version skip and the unwrap status filter were each broken in the production source, the corresponding tests went red, and the source was restored. A test that cannot fail protects nothing.
No production code is changed by this branch.
What could not be done, and why
Azure's mapping table cannot be asserted at the wire the way Vault's can: the SDK's transport hands the message handler a request whose content is already gone, so the algorithm actually sent is not observable through that seam. Left alone and recorded on the issue rather than worked around with a visibility change to production code.
Numbers
Azure's branch figure is unmoved because what remains there is the mapping table above.