Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ocsp: reject ocsp responses where version is not v1 #20406

Merged
merged 11 commits into from
Apr 13, 2022
Merged

ocsp: reject ocsp responses where version is not v1 #20406

merged 11 commits into from
Apr 13, 2022

Conversation

daixiang0
Copy link
Member

Signed-off-by: Loong Dai [email protected]

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA] #20096
[Optional Deprecated:]
[Optional API Considerations:]

@daixiang0
Copy link
Member Author

/wait

zuercher
zuercher reviewed Mar 24, 2022
View reviewed changes
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up -- I read through the documentation on ASN.1 tag classes and left some notes for you.

source/extensions/transport_sockets/tls/ocsp/ocsp.cc Outdated Show resolved Hide resolved
test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc Outdated Show resolved Hide resolved
@daixiang0
feedback
a58ae18 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
fix import
c62e5c0 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #20406 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0
fix tests
f5a100e 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
fix format
e19dd73 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
Copy link
Member Author

@zuercher thanks for your review! I have updated all and please take another review when free.

@daixiang0
feedback
84083ed 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
fix typo
c80fd6b 
Signed-off-by: Loong Dai <[email protected]>
@zuercher
Copy link
Member

zuercher commented Mar 30, 2022 •
edited
Loading

I'm concerned that this isn't actually correct still? It seems to me that the bitstring has to have a length associated with it, and we're not including that in our test. (I got that encoding from an online asn.1 encoder and a subset of the OCSP definition, which is why I'm not sure it's truly correct.) Can you do some research and see if you can confirm whether that's really what a different version would look like when encoded?

@zuercher
Copy link
Member

I'm also not available for the rest of the week.

@daixiang0
Copy link
Member Author

daixiang0 commented Mar 30, 2022 •
edited
Loading

I have tested some cases about bit string/version changes, I can confirm that it is right although not found any docs related to it, if change bit string, it would raise an error.

@daixiang0
remove useless field
3fafcc8 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
fix CI
6c5e0a4 
Signed-off-by: Loong Dai <[email protected]>
@zuercher
Copy link
Member

zuercher commented Apr 5, 2022

@davidben Do you have time to take a quick look at this and confirm that we're simulating an unsupported OCSP version correctly?

@daixiang0
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #20406 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0
feedback
bc7fdc3 
Signed-off-by: Loong Dai <[email protected]>
@zuercher
Copy link
Member

Belatedly realized that @ggreenway is out this week.

@zuercher zuercher merged commit ba362a2 into envoyproxy:main Apr 13, 2022
@daixiang0 daixiang0 deleted the ocsp branch April 14, 2022 00:47
@zuercher zuercher mentioned this pull request Apr 18, 2022
Closed
vehre-x41 pushed a commit to vehre-x41/envoy that referenced this pull request Apr 19, 2022
@daixiang0 @vehre-x41
ocsp: reject ocsp responses where version is not v1 (envoyproxy#20406)
06e749c 
Risk Level: low
Testing: added
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Loong Dai <[email protected]>

Signed-off-by: Andre Vehreschild <[email protected]>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
@daixiang0 @ravenblackx
ocsp: reject ocsp responses where version is not v1 (envoyproxy#20406)
8501ee0 
Risk Level: low
Testing: added
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Loong Dai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidben davidben davidben left review comments

@zuercher zuercher zuercher approved these changes

@lizan lizan Awaiting requested review from lizan

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

Assignees

@zuercher zuercher

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daixiang0 @zuercher @davidben @ggreenway