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
credentials: check and expose SPIFFE ID #3626
Conversation
@jiangtaoli2016 Can you please first take a look at the correctness of the plumbing flow and SPIFFE ID check? Thanks in advance! |
F.Y.I. https://github.com/spiffe/spiffe/blob/master/standards/SPIFFE-ID.md#23-maximum-spiffe-id-length this describes the maximum length of a SPIFFE ID. |
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.
Thanks Zhen for implementation! A minor comment.
66cd579
to
4ced5c9
Compare
4ced5c9
to
8d70904
Compare
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.
Thanks @ZhenLian for revision. A few minor comments.
We should also choose a Golang readability reviewer. |
PS: I am fine with not parsing SPIFFE ID for go version 1.9 and before -- for now. We may revisit this later. |
f0e61f5
to
7ed9175
Compare
7ed9175
to
3ed9ae7
Compare
@doug Since this change touches some part of tls pkg, could you review it please? Thanks in advance! |
Sorry it seems I @ ed the wrong person... @dfawley this change touches some part of tls package. could you please take a look at this please? Feel free to let me know if you have any concerns. |
credentials/gobefore10.go
Outdated
) | ||
|
||
func (t *TLSInfo) ParseSpiffeID() error { | ||
grpclog.Info("go version prior to 1.10 doesn't support parsing URIs in certificates. Please consider a newer version") |
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.
You can delete this; users don't need a log line every time they make a connection, and Go 1.9 isn't supported anyway.
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.
SG. I added a TODO above this function to remind me for the future deletion of this file. Please let me know if this works for you. Thanks!
af83b10
to
43a0b64
Compare
43a0b64
to
c993ac9
Compare
c993ac9
to
6625171
Compare
@dfawley This PR is ready again. Please let me know if it could be better improved. Thanks a lot for the review! |
credentials/credentials.go
Outdated
@@ -87,6 +88,7 @@ func (s SecurityLevel) String() string { | |||
// This API is experimental. | |||
type CommonAuthInfo struct { | |||
SecurityLevel SecurityLevel | |||
SPIFFEID *url.URL |
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 wouldn't consider SPIFFE to be "common" to all transport security implementations. I don't think it makes as much sense here, at least not if the only reason to move it was because this struct is already marked as experimental.
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.
Got it. I put it back to TLSInfo. Since TLSInfo itself is not EXPERIMENTAL, I marked only the SPIFFEID EXPERIMENTAL. Please let me know if it works:)
7cfc8a9
to
45a2729
Compare
@dfawley I'd like to provide some updates on this PR: |
45a2729
to
94e19d5
Compare
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.
Thanks much Zhen for quick revision!
@dfawley Hi Doug, this PR is ready for review again (sorry for the back and forth) :) |
This PR does the following three things:
SpiffeID
to the structTLSInfo
ParseSpiffeID
toTLSInfo
that will check the format of SPIFFE ID, and fill inif the check passes
ParseSpiffeID
at the end of ClientHandshake and ServerHandshake to fill theSpiffeID
After this change, users could use
to get the SPIFFE ID information(error handling omitted).
Note that parsing of URI fields is only supported in go version >= 1.10. For earlier versions, we log the error information and don't expose the field, and return a nil error.