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.

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

Add sct and cert verification #1

Merged
merged 1 commit into from May 11, 2022
Merged

Add sct and cert verification #1

merged 1 commit into from May 11, 2022

Conversation

loosebazooka
Copy link
Member

@loosebazooka loosebazooka commented Apr 15, 2022

Add sct and cert validations

  • Rename CertificateResponse to SigningCertificate
  • Add more testing around parsing
  • Add example fulcio/ctfe public keys and sct/cert examples for testing
  • Exceptions are still kinda just passed along

@loosebazooka loosebazooka force-pushed the add-sct-verification branch 2 times, most recently from b78b894 to 8bc998d Compare April 15, 2022 18:41
@loosebazooka
Copy link
Member Author

@di here is working sct/cert validation. There are sample in src/test/resources. They are generated off of my email, so it does feel weird to include them in a python client, but you could get away using them to test your code for now.

@loosebazooka loosebazooka force-pushed the add-sct-verification branch 2 times, most recently from 5b21366 to 1d72526 Compare May 4, 2022 00:28
@patflynn patflynn self-requested a review May 9, 2022 14:57
patflynn
patflynn previously approved these changes May 11, 2022
Copy link
Collaborator

@patflynn patflynn left a comment

Choose a reason for hiding this comment

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

Lots of mysterious stuff in here, but looks like you know what you're doing!

My meta comment is that this would be easier to review broken up into neater PRs. For instance the refactorings could have been separated out. No biggie though.

public CertificateResponse(CertPath certPath, @Nullable byte[] sct) {
this.certPath = certPath;
this.sct = sct;
public class FulcioValidationException extends Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's kind of weird to fork a response into an exception

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this extend RuntimeException?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we want the tool to catch this and present it to the user. I don't think it's a catastrophic failure. It could be a misconfigured private key, or the wrong fulcio or something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. Makes sense. We should chat about our approach here some time with Jason. I'll typically avoid throwing any checked exceptions unless it's type or api enables the client in some way.


public static FulcioValidator newFulcioValidator(
byte[] fulcioRoot, @Nullable byte[] ctfePublicKey)
throws InvalidKeySpecException, NoSuchAlgorithmException, CertificateException, IOException,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious why we allow the ctfePublicKjey to be null if that'll just result in a validation exception when we call validateSct?

Copy link
Member Author

Choose a reason for hiding this comment

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

it can be null because scts aren't required to be returned from fulcio. they should be, but people could be running private instances and maybe don't care about internal certificate transparency (but maybe they should?)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be enforced via a policy/config?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But validate will always fail as a result won't it?

- Rename CertificateResponse to SigningCertificate
- SCTs are optional
- Add more testing around parsing
- Add example fulcio/ctfe public keys and sct/cert examples for testing
- Exceptions are still kinda just passed along

Signed-off-by: Appu Goundan <appu@google.com>
Co-authored-by: Vladimir Sitnikov <sitnikov.vladimir@gmail.com>
@loosebazooka loosebazooka merged commit 217ec0a into main May 11, 2022
@loosebazooka loosebazooka deleted the add-sct-verification branch May 18, 2022 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants