Make sure to set uri_sans parameter #3971

hiyosi · 2023-03-13T07:56:26Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality

Description of change

Fixed a issue in which intermediate certificates issued by the HashiCorp Vault PKI engine do not contain a URI SAN.

Which issue this PR fixes

#3968

hiyosi · 2023-03-14T22:59:50Z

pkg/server/plugin/upstreamauthority/vault/vault_test.go

@@ -259,7 +259,7 @@ func TestMintX509CA(t *testing.T) {
 				},
 			},
 			authMethod:              TOKEN,
-			expectX509CA:            []string{"spiffe://intermediate-vault", "spiffe://intermediate"},
+			expectX509CA:            []string{"spiffe://intermediate-spire", "spiffe://intermediate-vault"},


Before this PR, values of URI SAN are little confusing.

amartinezfayo

Thank you @hiyosi for this contribution!
This is in draft mode, are you expecting to add more commits?

hiyosi · 2023-03-15T13:42:52Z

@amartinezfayo
I have only one concern.
I'm not sure a value of URI SAN(SPIFFE ID) must be set or not in X509SVID CA certificates.

amartinezfayo · 2023-03-17T12:13:43Z

I'm not sure a value of URI SAN(SPIFFE ID) must be set or not in X509SVID CA certificates.

While it would be desirable that a signing certificate itself should an SVID, it's not mandatory.
A more strict validation was introduced in SPIRE 1.6 that made this a hard requirement. After discussing this during our last maintainer's sync, we decided that it's better to relax that requirement so the process doesn't fail. There are chances that an upstream authority plugin can't control this, so we will change this to not have an unnecessary limitation there.

We still welcome this change though :)

amartinezfayo · 2023-03-17T17:42:31Z

pkg/server/plugin/upstreamauthority/vault/vault_client.go

+		uris = append(uris, uri.String())
+	}
+	if len(uris) == 0 {
+		return nil, status.Errorf(codes.Internal, "CSR must have least one URIs")


Since the error is due to an invalid input argument and not an internal failure I think that we should use codes.InvalidArgument.

Suggested change

return nil, status.Errorf(codes.Internal, "CSR must have least one URIs")

return nil, status.Errorf(codes.InvalidArgument, "CSR must have at least one URI")

fixed via a0ee7fd

amartinezfayo

Last final thing and we are ready to go. Sorry that I didn't suggest this in the first review.

amartinezfayo · 2023-03-21T13:00:22Z

pkg/server/plugin/upstreamauthority/vault/vault_client.go

+		uris = append(uris, uri.String())
+	}
+	if len(uris) == 0 {
+		return nil, status.Errorf(codes.InvalidArgument, "CSR must have least one URIs")


Suggested change

return nil, status.Errorf(codes.InvalidArgument, "CSR must have least one URIs")

return nil, status.Errorf(codes.InvalidArgument, "CSR must have at least one URI")

Fixed via 6bfee67

Signed-off-by: Tomoya Usami <[email protected]>

amartinezfayo

Thank you @hiyosi!

* Make sure to set uri_sans parameter Signed-off-by: Tomoya Usami <[email protected]> Signed-off-by: Basavaraju-G <[email protected]>

hiyosi force-pushed the specify-uri-sans branch 3 times, most recently from 8a7207f to 8daa8ed Compare March 13, 2023 09:50

amartinezfayo added this to the 1.6.2 milestone Mar 13, 2023

hiyosi mentioned this pull request Mar 14, 2023

Upstream Authority "vault" Plugin doesn't make X509 Certificate with SAN #3968

Closed

hiyosi commented Mar 14, 2023

View reviewed changes

hiyosi force-pushed the specify-uri-sans branch from 372de14 to 03becc4 Compare March 15, 2023 01:09

amartinezfayo self-assigned this Mar 15, 2023

amartinezfayo reviewed Mar 15, 2023

View reviewed changes

hiyosi marked this pull request as ready for review March 15, 2023 22:50

hiyosi requested review from evan2645, azdagron, MarcosDY and rturner3 as code owners March 15, 2023 22:50

amartinezfayo reviewed Mar 17, 2023

View reviewed changes

hiyosi force-pushed the specify-uri-sans branch from f9b6103 to a0ee7fd Compare March 17, 2023 22:48

amartinezfayo reviewed Mar 21, 2023

View reviewed changes

hiyosi force-pushed the specify-uri-sans branch from 5a59969 to f34b40b Compare March 21, 2023 22:58

hiyosi added 3 commits March 22, 2023 07:59

Make sure to set uri_sans parameter

ab0474a

Signed-off-by: Tomoya Usami <[email protected]>

fixup! Make sure to set uri_sans parameter

bc40ce7

Signed-off-by: Tomoya Usami <[email protected]>

fixup! fixup! Make sure to set uri_sans parameter

6bfee67

Signed-off-by: Tomoya Usami <[email protected]>

hiyosi force-pushed the specify-uri-sans branch from f34b40b to 6bfee67 Compare March 21, 2023 22:59

Merge branch 'main' into specify-uri-sans

5ebffcf

amartinezfayo approved these changes Mar 22, 2023

View reviewed changes

amartinezfayo merged commit f9c4b87 into spiffe:main Mar 22, 2023

hiyosi deleted the specify-uri-sans branch March 22, 2023 22:56

amartinezfayo modified the milestones: 1.6.2, 1.6.3 Apr 5, 2023

Basavaraju-G pushed a commit to Basavaraju-G/spire that referenced this pull request May 3, 2023

Make sure to set uri_sans parameter (spiffe#3971)

11f0413

* Make sure to set uri_sans parameter Signed-off-by: Tomoya Usami <[email protected]> Signed-off-by: Basavaraju-G <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make sure to set uri_sans parameter #3971

Make sure to set uri_sans parameter #3971

Uh oh!

hiyosi commented Mar 13, 2023 •

edited

Loading

Uh oh!

hiyosi Mar 14, 2023

Uh oh!

amartinezfayo left a comment

Uh oh!

hiyosi commented Mar 15, 2023 •

edited

Loading

Uh oh!

amartinezfayo commented Mar 17, 2023

Uh oh!

amartinezfayo Mar 17, 2023

Uh oh!

hiyosi Mar 17, 2023 •

edited

Loading

Uh oh!

amartinezfayo left a comment

Uh oh!

amartinezfayo Mar 21, 2023

Uh oh!

hiyosi Mar 21, 2023 •

edited

Loading

Uh oh!

amartinezfayo left a comment

Uh oh!

Uh oh!

	return nil, status.Errorf(codes.Internal, "CSR must have least one URIs")
	return nil, status.Errorf(codes.InvalidArgument, "CSR must have at least one URI")

Make sure to set uri_sans parameter #3971

Make sure to set uri_sans parameter #3971

Uh oh!

Conversation

hiyosi commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiyosi Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

amartinezfayo left a comment

Choose a reason for hiding this comment

Uh oh!

hiyosi commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amartinezfayo commented Mar 17, 2023

Uh oh!

amartinezfayo Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

hiyosi Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amartinezfayo left a comment

Choose a reason for hiding this comment

Uh oh!

amartinezfayo Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

hiyosi Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amartinezfayo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hiyosi commented Mar 13, 2023 •

edited

Loading

hiyosi commented Mar 15, 2023 •

edited

Loading

hiyosi Mar 17, 2023 •

edited

Loading

hiyosi Mar 21, 2023 •

edited

Loading