[Security] Add support for parsing SPIFFE Bundle Maps #8124
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8124 +/- ##
==========================================
+ Coverage 82.18% 82.42% +0.24%
==========================================
Files 387 392 +5
Lines 38947 39164 +217
==========================================
+ Hits 32007 32282 +275
+ Misses 5613 5567 -46
+ Partials 1327 1315 -12
🚀 New features to boost your workflow:
|
gtcooke94
added
Type: Security
| // use other than this is specified, the parser does not fail, it | ||
| // just doesn't add an x509 authority or jwt authority to the bundle | ||
| filePath: testdata.Path("spiffe/spiffebundle_wrong_use.json"), | ||
| expectNoX509: true, |
There was a problem hiding this comment.
Do we need this field in the table since all subtests are expected to fail, and therefore this field is always set to true.
There was a problem hiding this comment.
There's a slight variation in expectError and expectNoX509 that needs both of them
There was a problem hiding this comment.
This needs to be taken care of still.
There was a problem hiding this comment.
I see. Had a squint a bit to figure that out :)
Can we please move that out to a separate test to simplify the test logic in both of them. Please see: https://goto.google.com/gotip/episodes/50
| // use other than this is specified, the parser does not fail, it | ||
| // just doesn't add an x509 authority or jwt authority to the bundle | ||
| filePath: testdata.Path("spiffe/spiffebundle_wrong_use.json"), | ||
| expectNoX509: true, |
There was a problem hiding this comment.
This needs to be taken care of still.
| // use other than this is specified, the parser does not fail, it | ||
| // just doesn't add an x509 authority or jwt authority to the bundle | ||
| filePath: testdata.Path("spiffe/spiffebundle_wrong_use.json"), | ||
| expectNoX509: true, |
There was a problem hiding this comment.
There's a slight variation in expectError and expectNoX509 that needs both of them
| @@ -73,3 +78,40 @@ func SPIFFEIDFromCert(cert *x509.Certificate) *url.URL { | |||
| } | |||
| return spiffeID | |||
| } | |||
|
|
|||
| type partialParsedSPIFFEBundleMap struct { | |||
| Bundles map[string]json.RawMessage `json:"trust_domains"` | |||
There was a problem hiding this comment.
Nit: Sorry, didn't notice this earlier. But any specific reason for not naming it as TrustDomains to match the JSON field name?
| } | ||
| bundle, err := spiffebundle.Parse(trustDomain, jsonBundle) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("spiffe: failed to parse bundle in map file (%v): %v", filePath, err) |
There was a problem hiding this comment.
Nit: also please include the trust domain name here for a more specific error message. Thanks.
| // use other than this is specified, the parser does not fail, it | ||
| // just doesn't add an x509 authority or jwt authority to the bundle | ||
| filePath: testdata.Path("spiffe/spiffebundle_wrong_use.json"), | ||
| expectNoX509: true, |
There was a problem hiding this comment.
I see. Had a squint a bit to figure that out :)
Can we please move that out to a separate test to simplify the test logic in both of them. Please see: https://goto.google.com/gotip/episodes/50
| @@ -32,7 +32,7 @@ elif [[ "$#" -ne 0 ]]; then | |||
| fi | |||
|
|
|||
| # - Check that generated proto files are up to date. | |||
| go generate google.golang.org/grpc/... && git status --porcelain 2>&1 | fail_on_output || \ | |||
| go generate ./... && git status --porcelain 2>&1 | fail_on_output || \ | |||
There was a problem hiding this comment.
I think the complete fix would be to run go generate on all directories that contain a go.mod file, in addition to the root module.
This adds a dependency on go-spiffe in order to parse SPIFFE bundles. More specifically, that library does not yet support SPIFFE bundle maps, but it does support SPIFFE bundles. This adds parsing of these maps to grpc-go
See the gRFC for more detail grpc/proposal#462
RELEASE NOTES: N/A