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

_verify: Create a new X509Store for each verify call #174

Merged
merged 3 commits into from
Jul 28, 2022
Merged

_verify: Create a new X509Store for each verify call #174

merged 3 commits into from
Jul 28, 2022

Conversation

tetsuo-cpp
Copy link
Collaborator

Closes #173

@tetsuo-cpp
_verify: Create a new X509Store for each verify call
0e41a92 
Signed-off-by: Alex Cameron <alex.cameron@trailofbits.com>
@tetsuo-cpp
_verify: Trim comment to not pass column limit
52ec00a 
Signed-off-by: Alex Cameron <alex.cameron@trailofbits.com>
@tetsuo-cpp
Copy link
Collaborator Author

tetsuo-cpp commented Jul 28, 2022 •
edited

I think there might be an underlying issue to report to either pyopenssl or openssl itself since, if the set_time call succeeds, users would understandably expect it to use the new time. I'll figure out where I need to file a report.

In the meantime, let's just get it working again with this patch.

@woodruffw
Copy link
Member

I think there might be an underlying issue to report to either pyopenssl or openssl itself since, if the set_time call succeeds, users would understandably expect it to use the new time.

Yeah, that smells like a PyOpenSSL (or underlying OpenSSL) bug to me, or at least a documentation error.

I'd suggest filing it here: https://github.com/pyca/pyopenssl

woodruffw
woodruffw previously approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM!

tetsuo-cpp
tetsuo-cpp commented Jul 28, 2022
View reviewed changes
sigstore/_verify.py Outdated
store = X509Store()
for parent_cert_pem in self._fulcio_certificate_chain:
parent_cert = load_pem_x509_certificate(parent_cert_pem)
parent_cert_ossl = X509.from_cryptography(parent_cert)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it'd probably be better to do this in the constructor and just store the parent_cert_ossls for each chain member.

@tetsuo-cpp
_verify: Parse certificate chain in constructor to avoid duplicate work
95f7348 
Signed-off-by: Alex Cameron <alex.cameron@trailofbits.com>
@tetsuo-cpp
Copy link
Collaborator Author

Ok, I've removed the duplicate work. I'll let you take another look and pull the trigger on it.

woodruffw
woodruffw approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM structurally. We should probably get some tests in the CI that exercise this code path, so that we don't regress here in the future.

@woodruffw woodruffw merged commit 455b50d into main Jul 28, 2022
@woodruffw woodruffw deleted the alex/x509-time branch July 28, 2022 16:19
This was referenced Jul 28, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodruffw woodruffw woodruffw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verifying multiple files causes certificate is not yet valid from OpenSSL
2 participants
@tetsuo-cpp @woodruffw