credentials: check and expose SPIFFE ID #3626

ZhenLian · 2020-05-18T20:36:13Z

This PR does the following three things:

add a new field SpiffeID to the struct TLSInfo
add a function ParseSpiffeID to TLSInfo that will check the format of SPIFFE ID, and fill in
if the check passes
call ParseSpiffeID at the end of ClientHandshake and ServerHandshake to fill the SpiffeID

After this change, users could use

ctx context.Context
p, ok := peer.FromContext(ctx)
tlsInfo, ok := p.AuthInfo.(credentials.TLSInfo)

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.

ZhenLian · 2020-05-18T20:37:28Z

@jiangtaoli2016 Can you please first take a look at the correctness of the plumbing flow and SPIFFE ID check? Thanks in advance!

ZhenLian · 2020-05-18T20:39:31Z

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.

jiangtaoli2016

Thanks Zhen for implementation! A minor comment.

credentials/tls.go

jiangtaoli2016

Thanks @ZhenLian for revision. A few minor comments.

credentials/tls.go

credentials/go10.go

jiangtaoli2016 · 2020-05-26T21:13:13Z

We should also choose a Golang readability reviewer.

jiangtaoli2016 · 2020-05-26T21:16:45Z

PS: I am fine with not parsing SPIFFE ID for go version 1.9 and before -- for now. We may revisit this later.

credentials/go10.go

ZhenLian · 2020-06-02T05:29:27Z

@doug Since this change touches some part of tls pkg, could you review it please? Thanks in advance!

ZhenLian · 2020-06-11T16:42:49Z

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.
Thank you so much!

credentials/credentials_go10_test.go

credentials/go10.go

credentials/tls.go

credentials/go10.go

dfawley · 2020-06-11T21:33:06Z

credentials/gobefore10.go

+)
+
+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")


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.

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!

ZhenLian · 2020-06-15T04:03:35Z

@dfawley This PR is ready again. Please let me know if it could be better improved. Thanks a lot for the review!

dfawley · 2020-06-22T21:10:32Z

credentials/credentials.go

@@ -87,6 +88,7 @@ func (s SecurityLevel) String() string {
 // This API is experimental.
 type CommonAuthInfo struct {
 	SecurityLevel SecurityLevel
+	SPIFFEID      *url.URL


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.

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:)

internal/credentials/go110.go

ZhenLian · 2020-06-27T22:57:05Z

@dfawley I'd like to provide some updates on this PR:
We found that the envoy RBAC assumed that the SPIFFE ID is the first URI entry, because for a valid SPIFFE certificate, there would always be exactly one URI field with spiffe scheme. We'd like to keep alignment with this, so after the change, a certificate with IDs, say "spiffe://foo/bar" and "https://foo/bar" would be considered as invalid(similar changes in C core).
For invalid certs, we'd also like to not return errors, but just to log the errors and not plumb the ID.
@jiangtaoli2016 since this includes some logic changes(although not much), can you please take a look at the latest commit first? Thank you so much!

… just warning

internal/credentials/go110.go

jiangtaoli2016

Thanks much Zhen for quick revision!

ZhenLian · 2020-06-29T21:33:28Z

@dfawley Hi Doug, this PR is ready for review again (sorry for the back and forth) :)

credentials: check and expose SPIFFE ID

cd4b52f

ZhenLian requested a review from jiangtaoli2016 May 18, 2020 20:36

jiangtaoli2016 reviewed May 19, 2020

View reviewed changes

credentials/tls.go Outdated Show resolved Hide resolved

ZhenLian force-pushed the zhen_expose_spiffe_id branch from 66cd579 to 4ced5c9 Compare May 20, 2020 23:23

add build constraints to not support <= go 1.9

8d70904

ZhenLian force-pushed the zhen_expose_spiffe_id branch from 4ced5c9 to 8d70904 Compare May 20, 2020 23:25

ZhenLian requested a review from jiangtaoli2016 May 20, 2020 23:30

ZhenLian added the Type: Feature label May 21, 2020

dfawley assigned jiangtaoli2016 May 21, 2020

jiangtaoli2016 reviewed May 26, 2020

View reviewed changes

credentials/tls.go Outdated Show resolved Hide resolved

credentials/go10.go Outdated Show resolved Hide resolved

credentials/go10.go Outdated Show resolved Hide resolved

ZhenLian force-pushed the zhen_expose_spiffe_id branch from f0e61f5 to 7ed9175 Compare June 1, 2020 06:55

ZhenLian requested a review from jiangtaoli2016 June 1, 2020 06:55

jiangtaoli2016 approved these changes Jun 1, 2020

View reviewed changes

credentials/go10.go Outdated Show resolved Hide resolved

make test applicable only to >= go1.10

3ed9ae7

ZhenLian force-pushed the zhen_expose_spiffe_id branch from 7ed9175 to 3ed9ae7 Compare June 2, 2020 05:09

ZhenLian requested a review from dfawley June 2, 2020 05:29

dfawley self-assigned this Jun 11, 2020

dfawley reviewed Jun 11, 2020

View reviewed changes

ZhenLian force-pushed the zhen_expose_spiffe_id branch 6 times, most recently from af83b10 to 43a0b64 Compare June 15, 2020 00:06

ZhenLian force-pushed the zhen_expose_spiffe_id branch from 43a0b64 to c993ac9 Compare June 15, 2020 00:43

fix review comments; move funcs to internal

6625171

ZhenLian force-pushed the zhen_expose_spiffe_id branch from c993ac9 to 6625171 Compare June 15, 2020 03:34

ZhenLian requested a review from dfawley June 15, 2020 04:03

dfawley reviewed Jun 22, 2020

View reviewed changes

dfawley assigned ZhenLian and unassigned dfawley Jun 25, 2020

ZhenLian force-pushed the zhen_expose_spiffe_id branch from 7cfc8a9 to 45a2729 Compare June 27, 2020 22:47

ZhenLian requested a review from jiangtaoli2016 June 27, 2020 22:57

valid spiffe could only have one URI; don't return error for invalid,…

94e19d5

… just warning

ZhenLian force-pushed the zhen_expose_spiffe_id branch from 45a2729 to 94e19d5 Compare June 27, 2020 23:07

jiangtaoli2016 reviewed Jun 29, 2020

View reviewed changes

internal/credentials/go110.go Show resolved Hide resolved

internal/credentials/go110.go Outdated Show resolved Hide resolved

resolve jiangtao's comments

a988d1c

ZhenLian requested a review from jiangtaoli2016 June 29, 2020 21:04

jiangtaoli2016 approved these changes Jun 29, 2020

View reviewed changes

ZhenLian mentioned this pull request Jun 30, 2020

advancedtls: Add SNI logic to ServerOptions.GetCertificate #3697

Merged

ZhenLian requested a review from dfawley July 8, 2020 22:08

dfawley approved these changes Jul 8, 2020

View reviewed changes

dfawley added this to the 1.31 Release milestone Jul 8, 2020

ZhenLian merged commit dd8658f into grpc:master Jul 16, 2020

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

credentials: check and expose SPIFFE ID #3626

credentials: check and expose SPIFFE ID #3626

ZhenLian commented May 18, 2020 •

edited

ZhenLian commented May 18, 2020

ZhenLian commented May 18, 2020

jiangtaoli2016 left a comment

jiangtaoli2016 left a comment

jiangtaoli2016 commented May 26, 2020

jiangtaoli2016 commented May 26, 2020

ZhenLian commented Jun 2, 2020

ZhenLian commented Jun 11, 2020 •

edited

dfawley Jun 11, 2020

ZhenLian Jun 15, 2020

ZhenLian commented Jun 15, 2020

dfawley Jun 22, 2020

ZhenLian Jun 27, 2020

ZhenLian commented Jun 27, 2020

jiangtaoli2016 left a comment

ZhenLian commented Jun 29, 2020

credentials: check and expose SPIFFE ID #3626

credentials: check and expose SPIFFE ID #3626

Conversation

ZhenLian commented May 18, 2020 • edited

ZhenLian commented May 18, 2020

ZhenLian commented May 18, 2020

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

jiangtaoli2016 commented May 26, 2020

jiangtaoli2016 commented May 26, 2020

ZhenLian commented Jun 2, 2020

ZhenLian commented Jun 11, 2020 • edited

dfawley Jun 11, 2020

Choose a reason for hiding this comment

ZhenLian Jun 15, 2020

Choose a reason for hiding this comment

ZhenLian commented Jun 15, 2020

dfawley Jun 22, 2020

Choose a reason for hiding this comment

ZhenLian Jun 27, 2020

Choose a reason for hiding this comment

ZhenLian commented Jun 27, 2020

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

ZhenLian commented Jun 29, 2020

ZhenLian commented May 18, 2020 •

edited

ZhenLian commented Jun 11, 2020 •

edited