Skip to content

tls: fix daysUntilFirstCertExpires#20581

Merged
zuercher merged 34 commits intoenvoyproxy:mainfrom
daixiang0:spiffe
May 12, 2022
Merged

tls: fix daysUntilFirstCertExpires#20581
zuercher merged 34 commits intoenvoyproxy:mainfrom
daixiang0:spiffe

Conversation

@daixiang0
Copy link
Copy Markdown
Member

@daixiang0 daixiang0 commented Mar 30, 2022

Signed-off-by: Loong Dai loong.dai@intel.com

Commit Message: The root cause is that size_t is unsigned, and can thus only express positive numbers.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA] #20573
[Optional Deprecated:]
[Optional API Considerations:]

@daixiang0 daixiang0 requested a review from lizan as a code owner March 30, 2022 06:44
@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Mar 30, 2022

Another issue is that should we return a negative number for the day until first expired since it already expired?

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Mar 30, 2022

I feel that a real negative number is useless, just -1 is enough to mean it expired.

@daixiang0 daixiang0 requested a review from ggreenway as a code owner March 30, 2022 08:10
@daixiang0
tls: fix spiffe daysUntilFirstCertExpires
ccb1537 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
return -1 when expired
d4a0d5d 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@mkbehr mkbehr mentioned this pull request Mar 30, 2022
Closed
@daixiang0 daixiang0 changed the title tls: fix spiffe daysUntilFirstCertExpires tls: fix daysUntilFirstCertExpires Mar 31, 2022
adisuissa
adisuissa reviewed Mar 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@adisuissa adisuissa 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 fixing the bug!

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Apr 2, 2022

/wait for modifying

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Apr 7, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Apr 7, 2022

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

🐱

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

see: more, trace.

daixiang0 added 4 commits April 7, 2022 10:57
@daixiang0
modify return type
aabe78a 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
fix CI
68a62bc 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
update missing func
2f46bcb 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
update mock func
cde5fb3 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@adisuissa adisuissa self-assigned this Apr 8, 2022
daixiang0 added 4 commits April 8, 2022 09:58
@daixiang0
Merge branch 'main' into spiffe
c98cc33
@daixiang0
update value
92232e2 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
remove useless import
8d72e7b 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
fix null compare
0f96607 
Signed-off-by: Loong Dai <loong.dai@intel.com>
adisuissa
adisuissa reviewed Apr 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@adisuissa adisuissa 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 fixing this.

@adisuissa
Copy link
Copy Markdown
Contributor

adisuissa commented Apr 8, 2022

another nit: you can use make_optional to create an optional value.

@daixiang0
fix utility test
9f7adeb 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Apr 11, 2022

@adisuissa thanks for your review, I have updated all.

@daixiang0
feedback
2fce764 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
revert null
d85d2f8 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
update tests
06b3ae4 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
fix format
6bddf0e 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@zuercher
Copy link
Copy Markdown
Member

zuercher commented Apr 28, 2022 

C++ exception with description "JSON value from line 1 is larger than int64_t (not supported)" thrown in the test body.

I expect that's to do with your change. Not sure if the limit is in our code or a library.

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Apr 29, 2022 •
edited
Loading

It may be related to the that I change INT_MAX to SIZE_MAX.

@daixiang0
use int32_t rather than size_t
c3d3df2 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
update tests
bb9cbd1 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
fix CI
cf3a164 
Signed-off-by: Loong Dai <loong.dai@intel.com>
zuercher
zuercher reviewed Apr 29, 2022
View reviewed changes
daixiang0 added 3 commits May 5, 2022 09:30
@daixiang0
int32_t -> size_t
18491a5 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
fix CI
dacecb8 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
size_t -> uint32_t
9f4f684 
Signed-off-by: Loong Dai <loong.dai@intel.com>
zuercher
zuercher reviewed May 6, 2022
View reviewed changes
source/extensions/transport_sockets/tls/utility.h Outdated
* @return the number of days till this certificate is valid, the value is set when not expired.
*/
absl::optional<int32_t> getDaysUntilExpiration(const X509* cert, TimeSource& time_source);
absl::optional<size_t> getDaysUntilExpiration(const X509* cert, TimeSource& time_source);
Copy link
Copy Markdown
Member

@zuercher zuercher May 6, 2022

Choose a reason for hiding this comment

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

I think we should use uint32_t here as well?

Copy link
Copy Markdown
Member Author

@daixiang0 daixiang0 May 7, 2022

Choose a reason for hiding this comment

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

Sure.

zuercher
zuercher reviewed May 6, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Just one more comment.

daixiang0 added 4 commits May 7, 2022 08:33
@daixiang0
1
8e74e58 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
update Utility::getDaysUntilExpiration
4c66b5d 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
fix CI
b1f5ab4 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
fix CI
4e854e1 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented May 10, 2022

@zuercher please review again.

zuercher
zuercher reviewed May 10, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Looks good. Let's add release notes indicating the change and then we can merge.

daixiang0 added 3 commits May 11, 2022 08:08
@daixiang0
add release note
74cd906 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
Merge branch 'main' into spiffe
1c27f07
@daixiang0
update changelog
185afae 
Signed-off-by: Loong Dai <loong.dai@intel.com>
@zuercher zuercher merged commit 7fefd09 into envoyproxy:main May 12, 2022
@daixiang0 daixiang0 deleted the spiffe branch May 12, 2022 03:26
@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented May 25, 2022

I suspect there's an issue in this PR. Can we roll this back or quickly scrutinize it for unchecked accesses to optional values?

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented May 25, 2022

@jmarantz there is an issue, see the open fix PR here: #21428

If we want to do a quick revert here while that PR is sorted that is fine with me.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented May 25, 2022

Yan just pointed out #21428 to me, thanks.

I don't think we need a revert -- we can deal with this locally for now and wait for the proper fix in flight.

ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
@daixiang0 @ravenblackx
tls: fix daysUntilFirstCertExpires (envoyproxy#20581)
4475056 
The root cause is that size_t is unsigned, and can thus only express positive numbers.

Risk Level: low
Testing: unit tests updated
Docs Changes: n/a
Release Notes: added
Fixes envoyproxy#20573

Signed-off-by: Loong Dai <loong.dai@intel.com>
@phlax phlax mentioned this pull request Nov 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adisuissa adisuissa adisuissa left review comments

@mattklein123 mattklein123 mattklein123 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

@adisuissa adisuissa

Labels

api

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@daixiang0 @adisuissa @zuercher @jmarantz @mattklein123 @ggreenway