topdown/crypto: Add URIStrings field to JSON certs #6424
Conversation
charlieegan3
force-pushed
the
add-raw-uris-to-certs
branch
from
70a3a87
to
1f723a0
Compare
There was a problem hiding this comment.
@charlieegan3 it would be helpful if we could explain the need for this change in the commit message body. Thanks!
topdown/crypto.go
Outdated
| } | ||
| } | ||
|
|
||
| v, err := ast.InterfaceToValue(processedCerts) |
There was a problem hiding this comment.
We'd probably need something similar for crypto.x509.parse_and_verify_certificates ?
There was a problem hiding this comment.
Good catch, I've updated the implementation of this too in f71330c
topdown/crypto.go
Outdated
| // add a field to certs containing the URIs as strings | ||
| processedCerts := make([]struct { | ||
| x509.Certificate | ||
| RawURIs []string `json:"RawURIs"` |
There was a problem hiding this comment.
One comment about naming: I think the "raw" in RawURIs might give the impression that these are presented exactly as they appear in the subjectAltName extension. However, in general, passing a string through url.Parse and then URL.String will not always result in the original string.1
What do you think about naming this field URIStrings instead? (Or something else that doesn't have "raw" in the name?)
Footnotes
-
Here are a few pathological examples: https://go.dev/play/p/I3cnmN5W6dy. It's possible that this shouldn't ever matter if the subjectAltName extension conforms to all the requirements of RFC 5280 4.2.1.6, but I don't really understand this well enough to say for sure. ↩
There was a problem hiding this comment.
This is a good point, I've updated the field name in f71330c
charlieegan3
force-pushed
the
add-raw-uris-to-certs
branch
2 times, most recently
from
18a2c11
to
f71330c
Compare
ashutosh-narkar
force-pushed
the
add-raw-uris-to-certs
branch
from
f71330c
to
27daf83
Compare
charlieegan3
changed the title
charlieegan3
force-pushed
the
add-raw-uris-to-certs
branch
2 times, most recently
from
e09a139
to
f5363d3
Compare
test/cases/testdata/cryptox509parsecertificates/test-cryptox509parsecertificates-raw-uris.yaml
Show resolved
Hide resolved
test/cases/testdata/cryptox509parsecertificates/test-cryptox509parsecertificates-raw-uris.yaml
Show resolved
Hide resolved
charlieegan3
force-pushed
the
add-raw-uris-to-certs
branch
from
7f377bd
to
a0cd9dd
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
charlieegan3
force-pushed
the
add-raw-uris-to-certs
branch
from
a0cd9dd
to
ce62b35
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
charlieegan3
force-pushed
the
add-raw-uris-to-certs
branch
2 times, most recently
from
cf27335
to
08bee46
Compare
This is being added to make it easier to write policy on the contents of certificate URI SANs. This is where information like SPIFFE IDs etc are contained and it's helpful to Rego authors to have access to these values without rebuilding the URI from the parsed data under URIs. Fixes open-policy-agent#6416 Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Also, add a test for missing URI case. Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
ashutosh-narkar
force-pushed
the
add-raw-uris-to-certs
branch
from
08bee46
to
3805e5e
Compare
Fixes #6416, see issue for discussion.