Skip to content

tls: Add support for matching against OtherName SAN type#34471

Merged
ggreenway merged 24 commits intoenvoyproxy:mainfrom
arulthileeban:other_name_san
Jun 27, 2024
Merged

tls: Add support for matching against OtherName SAN type#34471
ggreenway merged 24 commits intoenvoyproxy:mainfrom
arulthileeban:other_name_san

Conversation

@arulthileeban
Copy link
Copy Markdown
Contributor

@arulthileeban arulthileeban commented Jun 2, 2024

Commit Message: tls: Add support for matching against OtherName SAN type

In the current SAN matcher, only DNS, URI, IP, EMAIL types are supported. This change adds support to match against OtherName. A new config field oid is added which helps define the type of OtherName SAN envoy needs to match against.

Risk Level: Low
Testing: Unit Testing
Docs Changes: Added
Release Notes: Added
Fixes #34358

arulthileeban and others added 5 commits June 1, 2024 13:41
@arulthileeban
tls: Add support for matching against OtherName SAN type
1c9c914 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@arulthileeban
tls: OtherName refactoring
d1f167f 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@arulthileeban
Merge branch 'envoyproxy:main' into other_name_san
7d9fa67
@arulthileeban
tls: Add tests and certs for OtherName SAN Validation
c95587b 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@arulthileeban
tls: Add docs for OtherName SAN matching
3aa7618 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 2, 2024

Hi @arulthileeban, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #34471 was opened by arulthileeban.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 2, 2024

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #34471 was opened by arulthileeban.

see: more, trace.

@arulthileeban
Copy link
Copy Markdown
Contributor Author

arulthileeban commented Jun 2, 2024

/retest

@arulthileeban
tls: Add UPN to spelling dictionary
27107f3 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
mattklein123
mattklein123 requested changes Jun 2, 2024
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.

High level question to get started, thanks.

/wait

soulxu
soulxu suggested changes Jun 3, 2024
View reviewed changes
@ggreenway ggreenway self-assigned this Jun 3, 2024
@arulthileeban
jwt_authn: Remove PANIC for invalid oid
fa8c908 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@arulthileeban
tls: Modify tests for othername SAN
8be3f1b 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
ggreenway
ggreenway reviewed Jun 4, 2024
View reviewed changes
@arulthileeban
tls: add logic to handle different othername type inputs from certs
5004b55 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
soulxu
soulxu reviewed Jun 5, 2024
View reviewed changes
soulxu
soulxu suggested changes Jun 5, 2024
View reviewed changes
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Jun 6, 2024

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Jun 6, 2024
@mattklein123 mattklein123 removed their assignment Jun 6, 2024
@arulthileeban
tls: Add Othername SAN value match docs and value failure test
543aa7e 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@repokitteh-read-only repokitteh-read-only bot added api and removed waiting labels Jun 6, 2024
ggreenway
ggreenway previously approved these changes Jun 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

I think this looks good.

I was thinking about the question of validating the type specifically or not, and I think it is probably ok not to because you are validating against a specific OID, and I think that is sufficient.

There's a merge conflict with the changelog; please merge main and fix the conflict.

/wait

@arulthileeban
Merge branch 'main' into other_name_san
90a980e 
Signed-off-by: Arul Thileeban Sagayam <arulthileeban@vt.edu>
@arulthileeban
Copy link
Copy Markdown
Contributor Author

arulthileeban commented Jun 27, 2024

Thanks for the review @ggreenway. Fixed the conflict and updated the branch

@ggreenway
Copy link
Copy Markdown
Member

ggreenway commented Jun 27, 2024

You have spelling failures in the format check; you can put backticks around words in comments to exclude them from spellcheck

@arulthileeban
add spelling fix
d3c7937 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@arulthileeban
Copy link
Copy Markdown
Contributor Author

arulthileeban commented Jun 27, 2024

You have spelling failures in the format check; you can put backticks around words in comments to exclude them from spellcheck

Thanks. Updated it

ggreenway
ggreenway requested changes Jun 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Sorry, one more fix: can you add test coverage for V_ASN1_BMPSTRING? I was looking at the coverage report (https://storage.googleapis.com/envoy-pr/868779a/coverage/source/common/tls/utility.cc.gcov.html) and that case is missed.

/wait

@arulthileeban
add bmpstring change
b1de297 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@arulthileeban
add bmpstring test+code
06d4848 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
@arulthileeban
Copy link
Copy Markdown
Contributor Author

arulthileeban commented Jun 27, 2024

Sorry, one more fix: can you add test coverage for V_ASN1_BMPSTRING? I was looking at the coverage report (https://storage.googleapis.com/envoy-pr/868779a/coverage/source/common/tls/utility.cc.gcov.html) and that case is missed.

/wait

My bad. I missed that one. Added the change required

ggreenway
ggreenway previously approved these changes Jun 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Thanks!

@ggreenway ggreenway enabled auto-merge (squash) June 27, 2024 21:39
@arulthileeban
bmpstring spelling fix
c773c79 
Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
auto-merge was automatically disabled June 27, 2024 21:52

Head branch was pushed to by a user without write access

@ggreenway ggreenway enabled auto-merge (squash) June 27, 2024 22:54
@ggreenway ggreenway merged commit aef38c4 into envoyproxy:main Jun 27, 2024
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@arulthileeban
tls: Add support for matching against OtherName SAN type (envoyproxy#…
4636a2b 
…34471)

In the current SAN matcher, only DNS, URI, IP, EMAIL types are supported. This change adds support to match against OtherName. A new config field oid is added which helps define the type of OtherName SAN envoy needs to match against.

Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@arulthileeban
tls: Add support for matching against OtherName SAN type (envoyproxy#…
0b8fe24 
…34471)

In the current SAN matcher, only DNS, URI, IP, EMAIL types are supported. This change adds support to match against OtherName. A new config field oid is added which helps define the type of OtherName SAN envoy needs to match against.

Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@arulthileeban
tls: Add support for matching against OtherName SAN type (envoyproxy#…
ca9439b 
…34471)

In the current SAN matcher, only DNS, URI, IP, EMAIL types are supported. This change adds support to match against OtherName. A new config field oid is added which helps define the type of OtherName SAN envoy needs to match against.

Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@arulthileeban
tls: Add support for matching against OtherName SAN type (envoyproxy#…
318e79e 
…34471)

In the current SAN matcher, only DNS, URI, IP, EMAIL types are supported. This change adds support to match against OtherName. A new config field oid is added which helps define the type of OtherName SAN envoy needs to match against.

Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
@arulthileeban arulthileeban mentioned this pull request Jul 18, 2024
Closed
@kindermoumoute kindermoumoute mentioned this pull request Aug 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggreenway ggreenway ggreenway approved these changes

@mattklein123 mattklein123 mattklein123 left review comments

+1 more reviewer

@soulxu soulxu soulxu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@soulxu soulxu

@ggreenway ggreenway

Labels

api

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow OtherName type in SAN matching

5 participants

@arulthileeban @mattklein123 @alyssawilk @ggreenway @soulxu