Introduce x509 DID method with PKI validation. #3446
Conversation
This file contains 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
Added support for x509 DID method including a new resolver and PKI validation. Adjusted metadata handling in key resolution across the codebase to include x509 specific fields and validation logic. - Added support for did:x509 as a new DID resolver. - Enabled validation of did:x509 VCs and VPs. - Added corresponding tests.
The previous code initialized a new variable instead of assigning to the existing 'err' variable. This change corrects the assignment, ensuring proper error handling during the DID resolution process.
Refactored resolver by moving X.509 related utilities to a new file. Added comprehensive validation for Subject Alternative Names (SANs). This enhances reliability and modularity.
Introduce specific error constants for `did:x509` parsing and validation failures. Refactor handling of PEM blocks, certificate chain validation, and policy checks to utilize these new error types and provide clearer messages.
Introduced functions to generate certificate chains and templates in `x509_test_utils.go` to aid in testing certificate-based operations. Additionally, added extensive resolver tests in `resolver_test.go` to validate various scenarios and ensure correct functionality.
Removed unnecessary closing braces from resolver_test.go to improve code readability and maintain a clean structure. This change does not affect the logic or functionality of the tests.
Extracted the logic for creating root, intermediate, and signing certificates into separate helper functions to reduce code complexity. Additionally, removed unused imports and the redundant DebugUnmarshall function to clean up the codebase.
reinkrul
requested changes
Oct 28, 2024
reinkrul
requested review from
woutslakhorst,
gerardsn and
stevenvegt
as code owners
Replaced explicit X509 header fields with a generic JwtProtectedHeaders map for better flexibility and reduced redundancy. Added the map to the metadata of all cases.
To prevent a name clash with the cert package.
Refactored the x509 DID resolver to introduce specific constants for subject and SAN policies, replacing magic strings. Implemented more detailed error handling and validation functions. Added extensive tests to cover new SAN and subject policy validation cases, ensuring robust handling of various attributes.
Extended the error handling in the resolver by distinguishing between SAN and subject policy errors. This provides clearer diagnosis and debugging for issues related to subject policies.
Replace custom unescapeValue function with url.QueryUnescape for more reliable URL decoding and error handling. Additionally, expand test cases to cover various scenarios involving malformed and partial URL inputs.
This update introduces two new subject policies: State (ST) and Street (STREET). The resolver now checks these policies, and corresponding tests have been added to validate the correct behavior. The test utilities have also been updated to include sample data for these fields.
Updated `x509_test_utils.go` and `resolver_test.go` to use generic, non-specific example domains and email addresses. This standardizes the test cases and avoids reliance on real-world domain names.
Renamed `forEachSAN` to `forEachSan` to maintain consistent camelCase naming convention across the codebase. This change affects the function definition, as well as all its invocations and documentation comments.
Co-authored-by: reinkrul <[email protected]>
Updated test names to provide clearer, more specific descriptions of each test's focus. This improves code readability and helps identify test purposes at a glance.
Implemented a new utility function `ExtractProtectedHeaders` for extracting protected headers from JWTs in the `didx509` package. Refactored the `verifier` package to use this new utility, enhancing code reuse and maintainability.
Added input validation to the ExtractProtectedHeaders function to ensure only valid JWT strings are processed. Updated the test cases to match the new expected behavior. This change helps prevent errors and makes the function more robust.
Separated subject and SAN policy validation into distinct functions. This improves code readability and facilitates easier maintenance by isolating the logic for each type of policy validation. Also, extracted SAN alternative name handling to a dedicated function for better structure.
gerardsn
reviewed
Oct 29, 2024
gerardsn
reviewed
Oct 29, 2024
This commit introduces `validation.go` to handle X.509 certificate validation based on specific policies. It moves existing validation logic from `resolver.go` to `validation.go`, consolidating it and making `ValidatePolicy` a shared function. Additionally, unit tests are updated to reflect these changes.
Co-authored-by: Gerard Snaauw <[email protected]>
Corrected the order of certificates in the chain array to reflect the accurate hierarchy for validation. Adjusted the "kid" header to fix a failing test introduced by the #0 added to the verification method id.
Extracted the logic for extracting SAN values into a separate function `findSanValue`. This improves code readability and modularity, facilitating easier maintenance and potential future enhancements.
gerardsn
requested changes
Oct 30, 2024
There was a problem hiding this comment.
Very limited review since there is no reference to the spec + version being implemented.
Some general comments:
- files in
vdr/didx509need more comments. I currently have no information on choices that were made to maintain this in the future. - seems to be missing a bunch of non-happy flow tests, but the code coverage tool is also marking things untested that are tested... The truth is probably somewhere in between.
Refactored the `ExtractProtectedHeaders` function into a variable for easier mocking in tests. Added a new test case to ensure the verifier correctly handles failures when `ExtractProtectedHeaders` returns an error.
This commit introduces multiple tests to verify x509 credentials, including cases for valid, revoked, untrusted, expired, and credentials with broken headers. The test suite aims to ensure comprehensive validation and error handling for the x509 credential verification process.
Implemented error handling to ensure JWTs have exactly one signature. Updated corresponding tests to include scenarios with valid, double, and no signatures.
Introduce new error types `ErrNoCertsInHeaders` and `ErrNoMatchingHeaderCredentials` to handle specific cases of missing or mismatched JWT protected headers. Updated tests to cover scenarios where these headers are absent.
Removed a redundant comment in ExtractProtectedHeaders pertaining to JWT string validation. The function already specifies that parsing errors are ignored and an empty map is returned.
Updated various test cases to use assert.ErrorIs and assert.EqualError for better error matching and clarity. This ensures the tests are more accurate and readable.
Replaced `assert.Equal` with `assert.EqualError` in resolver tests to improve error message assertions. This change ensures a more precise comparison of error messages in the test cases.
Removed redundant .Return(nil) calls from validator's Validate method expectations across various resolver tests. This cleanup simplifies the code and clarifies the intent of the tests without altering their functionality.
|
This is missing an e2e-test. Certificate revocation is currently not checked. The An e2e-test should show
|
Refactor error handling in x509 utilities to use custom error variables, improving clarity. Added comprehensive test cases for functions including certificate parsing, SAN processing, and certificate chain building, enhancing coverage and robustness.
Implemented tests for the ResolveMetadata struct to verify the behavior of GetProtectedHeaderString and GetProtectedHeaderChain methods. This ensures correct functionality for different input scenarios, including cases where headers do not exist or values are of incorrect types.
* master: (72 commits) PKI add ValidateStrict (#3531) PEX: Return capture group for matched patterns (#3526) Schedule CodeQL twice a week (#3525) change cron schedule (#3524) Add gh action for CodeQL schedule (#3523) Bump github.com/lestrrat-go/jwx/v2 from 2.1.1 to 2.1.2 (#3520) docs: v6 release date (#3519) status codes for discovery client (#3513) Require SQL connection string in strictmode (#3517) Fix duplicate discovery results (#3515) Bump github.com/chromedp/chromedp from 0.11.0 to 0.11.1 (#3514) fix duplicate search results for wildcard param (#3512) Bump github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys (#3511) remove network migration and optimize network event retry (#3510) secure outgoing http client with max connections (#3508) make gen-mocks (#3509) Bump github.com/Azure/azure-sdk-for-go/sdk/azcore from 1.15.0 to 1.16.0 (#3506) fix invalid keyReference migration objects (#3504) Bump go.uber.org/mock from 0.4.0 to 0.5.0 (#3507) Bump github.com/nats-io/nats-server/v2 from 2.10.21 to 2.10.22 (#3505) ... # Conflicts: # vcr/test.go # vdr/legacy_integration_test.go # vdr/vdr.go # vdr/vdr_test.go
Replaced calls to Validate with ValidateStrict to enhance the strictness of PKI validation across the validation of x509. This change ensures more strict validation checks are applied for the x509 credentials.
reinkrul
approved these changes
Nov 3, 2024
woutslakhorst
requested changes
Nov 4, 2024
This commit includes the GPLv3 license headers at the beginning of all source files in the `vdr/didx509` directory. This ensures legal and licensing clarity for the project's usage and distribution.
Detailed descriptions were added for constants, error messages, and functions across multiple files for better code readability and maintainability. These comments provide context and explanations for the defined elements, making the intent and usage clearer for future developers and documentation purposes.
Refactored the ValidatePolicy function to lowercase the initial letter, adhering to Go's convention for unexported functions.
…dHeaderChain Added documentation for both GetProtectedHeaderString and GetProtectedHeaderChain methods in ResolveMetadata to describe workings of these methods.
Documented that the resolve method to adheres to the DID:x509 v1.0 Draft specification. Notable changes include the implementation of the "otherName" SAN policy and support for "serialNumber" in the "subject" policy. The "eku" policy remains unimplemented.
This commit moves the ExtractProtectedHeaders to the crypto package where it located together with other JWS/JWT related methods.
woutslakhorst
approved these changes
Nov 5, 2024
gerardsn
approved these changes
Nov 5, 2024
reinkrul
reviewed
Nov 5, 2024
| // findSanValue extracts the SAN value from a given pkix.Extension, returning the resulting value or an error. | ||
| func findSanValue(extension pkix.Extension) (string, error) { | ||
| value := "" | ||
| err := forEachSan(extension.Value, func(data []byte) error { |
There was a problem hiding this comment.
I'm thinking, can't you have multiple otherName entries in the SAN (or any other SAN)? Now it only supports the first one?
There was a problem hiding this comment.
Well, the last one, actually. I'll take a look at this.
stevenvegt
requested changes
Nov 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Added support for x509 DID method including a new resolver and PKI validation. Adjusted metadata handling in key resolution across the codebase to include x509 specific fields and validation logic.