-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Parse and verify certificates with options #6583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
e362e13
23ceed5
35d2f85
a1f123d
4a472ff
290918c
c91d9cd
6ba2619
a8ba1d1
9c655c3
6b32620
4f240dd
4597ffe
6556e4e
4562893
2fe697c
ce57d7a
124047c
444388d
8cc5838
ee1819b
797e401
4a40db9
d6331e8
9fffee2
9bb2127
6d47ba4
5303e09
6066aae
5eda48e
c8422fe
aa87a3a
0436522
74b6730
458e963
dc6683a
a9b4bd8
720b333
f992c0d
9effeab
ea77470
3c31aa6
580fa87
f9b196d
5cad579
49796ad
8ebe57a
ce4a226
3993618
91e9007
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -207,6 +207,7 @@ var DefaultBuiltins = [...]*Builtin{ | |||||
| // Crypto | ||||||
| CryptoX509ParseCertificates, | ||||||
| CryptoX509ParseAndVerifyCertificates, | ||||||
| CryptoX509ParseAndVerifyCertificatesWithOptions, | ||||||
| CryptoMd5, | ||||||
| CryptoSha1, | ||||||
| CryptoSha256, | ||||||
|
|
@@ -2327,6 +2328,51 @@ with all others being treated as intermediates.`, | |||||
| ), | ||||||
| } | ||||||
|
|
||||||
| var CryptoX509ParseAndVerifyCertificatesWithOptions = &Builtin{ | ||||||
| Name: "crypto.x509.parse_and_verify_certificates_with_options", | ||||||
| Description: `Returns one or more certificates from the given string containing PEM | ||||||
| or base64 encoded DER certificates after verifying the supplied certificates form a complete | ||||||
| certificate chain back to a trusted root. Second argument is a option object to pass | ||||||
| extra configs to verify the validity of certificates. | ||||||
|
||||||
|
|
||||||
| The first certificate is treated as the root and the last is treated as the leaf, | ||||||
| with all others being treated as intermediates.` + | ||||||
|
||||||
| "Example option object: " + | ||||||
| "`options := {\"DNSName\" : \"example.dns.com\", \"CurrentTime\": 1708447636000000000,`" + | ||||||
| "`\"KeyUsages\": {\"KeyUsageServerAuth\", \"KeyUsageClientAuth\", \"KeyUsageCodeSigning\"},`" + | ||||||
| "`\"MaxConstraintComparisons\" : 5,`" + | ||||||
| "`}`" + | ||||||
| "Then this function call will look like: `crypto.x509.parse_and_verify_certificates_with_options(<cert base64 encoded string>, options)`" + | ||||||
| "This option's field has same definitions as fields in [x509.VerifyOptions struct](https://pkg.go.dev/crypto/x509#VerifyOptions)" + | ||||||
|
||||||
| "This option's field has same definitions as fields in [x509.VerifyOptions struct](https://pkg.go.dev/crypto/x509#VerifyOptions)" + | |
| "This options object has the same fields as [x509.VerifyOptions](https://pkg.go.dev/crypto/x509#VerifyOptions)" + |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "`CurrentTime` is nanoseconds since epoch" + | |
| "`CurrentTime` is the number of nanoseconds since the Unix Epoch as a number" + |
charlieegan3 marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,7 @@ import ( | |||||
| "hash" | ||||||
| "os" | ||||||
| "strings" | ||||||
| "time" | ||||||
|
|
||||||
| "github.com/open-policy-agent/opa/internal/jwx/jwk" | ||||||
|
|
||||||
|
|
@@ -104,7 +105,7 @@ func builtinCryptoX509ParseAndVerifyCertificates(_ BuiltinContext, operands []*a | |||||
| return iter(invalid) | ||||||
| } | ||||||
|
|
||||||
| verified, err := verifyX509CertificateChain(certs) | ||||||
| verified, err := verifyX509CertificateChain(certs, x509.VerifyOptions{}) | ||||||
| if err != nil { | ||||||
| return iter(invalid) | ||||||
| } | ||||||
|
|
@@ -122,6 +123,130 @@ func builtinCryptoX509ParseAndVerifyCertificates(_ BuiltinContext, operands []*a | |||||
| return iter(valid) | ||||||
| } | ||||||
|
|
||||||
| var allowedKeyUsages = map[string]x509.ExtKeyUsage{ | ||||||
| "KeyUsageAny": x509.ExtKeyUsageAny, | ||||||
| "KeyUsageServerAuth": x509.ExtKeyUsageServerAuth, | ||||||
| "KeyUsageClientAuth": x509.ExtKeyUsageClientAuth, | ||||||
| "KeyUsageCodeSigning": x509.ExtKeyUsageCodeSigning, | ||||||
| "KeyUsageEmailProtection": x509.ExtKeyUsageEmailProtection, | ||||||
| "KeyUsageIPSECEndSystem": x509.ExtKeyUsageIPSECEndSystem, | ||||||
| "KeyUsageIPSECTunnel": x509.ExtKeyUsageIPSECTunnel, | ||||||
| "KeyUsageIPSECUser": x509.ExtKeyUsageIPSECUser, | ||||||
| "KeyUsageTimeStamping": x509.ExtKeyUsageTimeStamping, | ||||||
| "KeyUsageOCSPSigning": x509.ExtKeyUsageOCSPSigning, | ||||||
| "KeyUsageMicrosoftServerGatedCrypto": x509.ExtKeyUsageMicrosoftServerGatedCrypto, | ||||||
| "KeyUsageNetscapeServerGatedCrypto": x509.ExtKeyUsageNetscapeServerGatedCrypto, | ||||||
| "KeyUsageMicrosoftCommercialCodeSigning": x509.ExtKeyUsageMicrosoftCommercialCodeSigning, | ||||||
| "KeyUsageMicrosoftKernelCodeSigning": x509.ExtKeyUsageMicrosoftKernelCodeSigning, | ||||||
| } | ||||||
|
|
||||||
| func builtinCryptoX509ParseAndVerifyCertificatesWithOptions(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error { | ||||||
|
|
||||||
| input, err := builtins.StringOperand(operands[0].Value, 1) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
yogisinha marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| options, err := builtins.ObjectOperand(operands[1].Value, 2) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| invalid := ast.ArrayTerm( | ||||||
| ast.BooleanTerm(false), | ||||||
| ast.NewTerm(ast.NewArray()), | ||||||
| ) | ||||||
|
|
||||||
| certs, err := getX509CertsFromString(string(input)) | ||||||
| if err != nil { | ||||||
| return iter(invalid) | ||||||
| } | ||||||
|
|
||||||
| // Collect the cert verification options | ||||||
| verifyOpt, err := extractVerifyOpts(options) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| verified, err := verifyX509CertificateChain(certs, verifyOpt) | ||||||
| if err != nil { | ||||||
| return iter(invalid) | ||||||
| } | ||||||
|
|
||||||
| value, err := ast.InterfaceToValue(verified) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| valid := ast.ArrayTerm( | ||||||
| ast.BooleanTerm(true), | ||||||
| ast.NewTerm(value), | ||||||
| ) | ||||||
|
|
||||||
| return iter(valid) | ||||||
| } | ||||||
|
|
||||||
| func extractVerifyOpts(options ast.Object) (verifyOpt x509.VerifyOptions, err error) { | ||||||
|
|
||||||
| for _, key := range options.Keys() { | ||||||
| switch key.String() { | ||||||
| case "\"DNSName\"": | ||||||
|
||||||
| dns, ok := options.Get(key).Value.(ast.String) | ||||||
| if ok { | ||||||
| verifyOpt.DNSName = dns.String()[1 : len(dns.String())-1] | ||||||
|
||||||
| verifyOpt.DNSName = dns.String()[1 : len(dns.String())-1] | |
| verifyOpt.DNSName = strings.Trim(string(dns), "\"") |
yogisinha marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might we want to support the case where the user supplies an array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided on Set because it automatically dedups the Key usage entry if user has supplied same entry more than once. Array would not do that. LMK what you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just imagining that a user might provide an array by mistake, if it were coming from input or data loaded from JSON this is quite likely. I agree that a Set is what we'd like to receive in an ideal case, but I think there's merit in covering an array as input too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. I would change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having dup in array is ok. its not a issue. I looked into Go code and passing list with dup key usages should be fine. just extra loop executions in this part of code: https://cs.opensource.google/go/go/+/refs/tags/go1.22.0:src/crypto/x509/verify.go;l=1166
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the Array type for KeyUsages.

Uh oh!
There was an error while loading. Please reload this page.