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

Add support for OCSP verification #1213

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Add support for OCSP verification #1213

merged 1 commit into from
Jan 10, 2023

Conversation

fnikolai
Copy link
Contributor

Signed-off-by: Fotis Nikolaidis [email protected]

Description of the Pull Request (PR):

Add support for Online Revocation Checks using the OCSP protocols.

This fixes or addresses the following GitHub issues:

Testing & Limitations

Since there is no officially signed certificate chain for Singularity, the validation of OCSP is done:

  1. Using the self-signed certificates. However, e2e testing fails because the root certificate is signed using Ed25519 (see the issue discussion)

  2. Using third-party certificate chain (AKAMAI). OCSP successfully validates the certificate, but the signature verification fails (since they are not useful for signing the image).

@fnikolai fnikolai force-pushed the ocsp-support branch 2 times, most recently from d6890c9 to 3081ce6 Compare December 26, 2022 14:09
@fnikolai fnikolai mentioned this pull request Dec 26, 2022
Closed
@dtrudg dtrudg requested a review from tri-adam December 28, 2022 16:27
dtrudg
dtrudg previously requested changes Dec 28, 2022
View reviewed changes
Copy link
Member

@dtrudg dtrudg left a comment

Choose a reason for hiding this comment

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

Hi @fnikolai, and many thanks again for your continued work on this!

I've left some more general comments on the code. I'm sure that @tri-adam will be along soon to go over the implementation in more detail.

Let me know if there are any questions.

e2e/verifyocsp/responder/standalone/add_cert_to_index.sh Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cmd/internal/cli/verify.go Outdated Show resolved Hide resolved
e2e/suite.go Outdated Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Outdated Show resolved Hide resolved
internal/app/singularity/verify.go Outdated Show resolved Hide resolved
internal/app/singularity/verify.go Outdated Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Outdated Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Outdated Show resolved Hide resolved
tri-adam
tri-adam reviewed Dec 28, 2022
View reviewed changes
Copy link
Member

@tri-adam tri-adam left a comment

Choose a reason for hiding this comment

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

@fnikolai, looking good! Left some comments/questions. Thank you!

test/certs/gen_certs.go Outdated Show resolved Hide resolved
test/certs/gen_certs.go Outdated Show resolved Hide resolved
test/certs/gen_certs.go Outdated Show resolved Hide resolved
test/certs/gen_certs.go Outdated Show resolved Hide resolved
cmd/internal/cli/verify.go Outdated Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Outdated Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Outdated Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Outdated Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Outdated Show resolved Hide resolved
internal/app/singularity/verify_ocsp.go Outdated Show resolved Hide resolved
@fnikolai fnikolai force-pushed the ocsp-support branch 6 times, most recently from f001453 to 49bb33b Compare December 29, 2022 14:17
@fnikolai fnikolai requested review from dtrudg and tri-adam and removed request for dtrudg and tri-adam December 30, 2022 09:35
@dtrudg
Copy link
Member

dtrudg commented Jan 3, 2023

Thanks for the updates @fnikolai - it'll likely get re-reviewed toward the end of this week / early next week as people return from holidays.

@tri-adam
Copy link
Member

tri-adam commented Jan 5, 2023

Just returning from holidays here, thank you for your patience @fnikolai! Replies added directly inline.

@dtrudg dtrudg dismissed their stale review January 6, 2023 09:19

Defer to tri-adam's review

@fnikolai fnikolai force-pushed the ocsp-support branch 3 times, most recently from b7ac1cd to a41721e Compare January 7, 2023 13:09
tri-adam
tri-adam previously requested changes Jan 9, 2023
View reviewed changes
Copy link
Member

@tri-adam tri-adam 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 the last remaining issue that I can see.

I'm conscious that the E2E tests are using a cert chain that is from a TLS client/server, and that removing this will break that test. For now, I think it might be best to remove that test, or assert that it fails, so this work can be merged.

internal/app/singularity/verify.go Outdated Show resolved Hide resolved
@dtrudg
Copy link
Member

dtrudg commented Jan 9, 2023

Looks like this will be mergeable soon, @tri-adam / @fnikolai ? I'll add it to the 3.11 milestone now on that assumption.

Please let me know if I'm wrong on this. Thanks!

@dtrudg dtrudg added this to the SingularityCE 3.11 milestone Jan 9, 2023
@fnikolai
Copy link
Contributor Author

fnikolai commented Jan 9, 2023

... For now, I think it might be best to remove that test, or assert that it fails, so this work can be merged.

@tri-adam x509.ExtKeyUsageClientAuth is removed and the proper assertion is added to the test.

Looks like this will be mergeable soon, @tri-adam / @fnikolai ? I'll add it to the 3.11 milestone now on that assumption.

Please let me know if I'm wrong on this. Thanks!

@dtrudg I believe it is now ready to be merged.

@dtrudg
Copy link
Member

dtrudg commented Jan 9, 2023

@dtrudg I believe it is now ready to be merged.

There's a conflict on go.mod due to the changes that have happened on the main branch recently. If you are able, could you rebase onto main? Otherwise if the box is checked to allow maintainers to modify the PR branch, I can take care of that after @tri-adam indicates he's happy.

Thanks!

@fnikolai
Add support for OCSP verification
6f3a862 
Signed-off-by: Fotis Nikolaidis <[email protected]>
@fnikolai
Copy link
Contributor Author

fnikolai commented Jan 9, 2023

@dtrudg done

@dtrudg dtrudg dismissed tri-adam’s stale review January 10, 2023 08:29

Points raised have been addressed.

@dtrudg
Copy link
Member

dtrudg commented Jan 10, 2023

@fnikolai - LGTM. I'll do a final quick sanity check for my benefit then merge, as tri-adam is travelling.

Many thanks for your efforts on this, and patience working through the reviews etc. over the holiday period.

@dtrudg dtrudg merged commit 0282c9d into sylabs:main Jan 10, 2023
@fnikolai
Copy link
Contributor Author

@dtrudg I believe it is now ready to be merged.

There's a conflict on go.mod due to the changes that have happened on the main branch recently. If you are able, could you rebase onto main? Otherwise if the box is checked to allow maintainers to modify the PR branch, I can take care of that after @tri-adam indicates he's happy.

Thanks!

It's my pleasure to jump in. Looking forward to the next contribution.

PS, I believe #1152 and perhaps #1150 can be closed.

@edytuk edytuk mentioned this pull request Mar 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtrudg dtrudg dtrudg approved these changes

@tri-adam tri-adam tri-adam left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
SingularityCE 3.11
Development

Successfully merging this pull request may close these issues.
3 participants
@fnikolai @dtrudg @tri-adam