Skip to content

tls: enable match_subject_alt_names option in SPIFFE validator#15509

Merged
mattklein123 merged 9 commits intoenvoyproxy:mainfrom
mathetake:spiffe-san-match
Mar 22, 2021
Merged

tls: enable match_subject_alt_names option in SPIFFE validator#15509
mattklein123 merged 9 commits intoenvoyproxy:mainfrom
mathetake:spiffe-san-match

Conversation

@mathetake
Copy link
Copy Markdown
Member

@mathetake mathetake commented Mar 16, 2021

Signed-off-by: Takeshi Yoneda [email protected]

Commit Message: tls: enable match_subject_alt_names option in SPIFFE validator
Additional Description: This is a follow up on #14884 and resolves #15392.

@mathetake mathetake mentioned this pull request Mar 16, 2021
Closed
@mathetake
Fix typos
157df74 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Fix heap-use-after-free in test
0338abf 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake mathetake marked this pull request as ready for review March 16, 2021 13:06
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Mar 16, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Mar 16, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15509 (comment) was created by @mathetake.

see: more, trace.

azdagron
azdagron reviewed Mar 16, 2021
View reviewed changes
Copy link
Copy Markdown

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exciting! Very happy to see this work move forward! I can't speak to the rest of the mechanics, but the actual SAN validation seems fine.

@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Mar 17, 2021

@azdagron Thanks for the review from the perspective of SPIFFE expert 👍 I really appreciate it 🙂

asraa
asraa reviewed Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM besides the nit

@mathetake mathetake requested a review from PiotrSikora as a code owner March 18, 2021 00:10
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Mar 18, 2021 •
edited
Loading

@PiotrSikora sorry my stale merge commit has requested your review. Never mind.

@mathetake
Fix build
d598d05 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Mar 18, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Mar 18, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15509 (comment) was created by @mathetake.

see: more, trace.

asraa
asraa previously approved these changes Mar 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM
@envoyproxy/senior-maintainers

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Mar 19, 2021

Needs a main merge.

/wait

@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Mar 19, 2021

merged main and resolved conflicts. Thanks!

mattklein123
mattklein123 reviewed Mar 19, 2021
View reviewed changes
source/extensions/transport_sockets/tls/utility.cc
Comment on lines +149 to +153
case GEN_EMAIL: {
ASN1_STRING* str = general_name->d.rfc822Name;
san.assign(reinterpret_cast<const char*>(ASN1_STRING_data(str)), ASN1_STRING_length(str));
break;
}
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have explicit testing somewhere? I don't see any cert changes in this PR?

/wait-any

Copy link
Copy Markdown
Member Author

@mathetake mathetake Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this here for testing SAN matcher https://github.com/envoyproxy/envoy/pull/15509/files#diff-ee57e8aee637e17f1f512cef5e2bff807c263ab67922c85659721654913242f4R460, so that's why you don't see certs changes.

In any way, generalNameAsString doesn't have any explicit unit test so I think we should. Added a dedicated test case of getSubjectAltNames for email SAN.

@mattklein123 mattklein123 self-assigned this Mar 19, 2021
@mathetake
Email SAN test
288740c 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Add integration tests
208482d 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Mar 22, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Mar 22, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15509 (comment) was created by @mathetake.

see: more, trace.

mattklein123
mattklein123 approved these changes Mar 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super, thanks for beefing up the test coverage!

@mattklein123 mattklein123 merged commit e39d2b3 into envoyproxy:main Mar 22, 2021
@mathetake mathetake deleted the spiffe-san-match branch March 22, 2021 23:45
This was referenced Mar 23, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@ggreenway ggreenway Awaiting requested review from ggreenway ggreenway is a code owner

@lizan lizan Awaiting requested review from lizan

@PiotrSikora PiotrSikora Awaiting requested review from PiotrSikora

+2 more reviewers

@azdagron azdagron azdagron left review comments

@asraa asraa asraa left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@asraa asraa

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tls: SAN matching support in SPIFFE certificate validator

4 participants

@mathetake @mattklein123 @asraa @azdagron