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

LG-3043: map bridge certs #154

Merged
merged 12 commits into from
Jun 18, 2020
Merged

LG-3043: map bridge certs #154

merged 12 commits into from
Jun 18, 2020

Conversation

solipet
Copy link
Contributor

@solipet solipet commented Jun 16, 2020

Significantly reduces the number of root CA's required and ensures that we don't end up in an infinite loop when building up the signatory chain.

@solipet solipet requested review from jmhooper and achapm June 16, 2020 16:24
@@ -18,6 +18,7 @@ def chain(set = [])
@chain ||= begin
signer = store[certificate.signing_key_id]
while signer
break if set.include? signer
Copy link
Contributor

Choose a reason for hiding this comment

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

is signer a key or a whole certificate object? IIRC certificate objects are like instance comparisons in ruby so they are rarely == to each other, it's better to compare some sort of ID?

Copy link
Member

@jmhooper jmhooper Jun 18, 2020

Choose a reason for hiding this comment

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

Our Certificate class actually overrides ==. It should be safe to compare 2 Certificate objects, but not 2 OpenSSL::X509::Certificate objects.

Ref: https://github.com/18F/identity-pki/blob/master/app/models/certificate.rb#L37-L41

Copy link
Contributor

Choose a reason for hiding this comment

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

oh perfect, thanks for that!

@solipet solipet force-pushed the dprice-lg-3043-map-bridge-certs branch from 692c7cf to 9755691 Compare June 17, 2020 14:48
# DoD root identifiers:
# 49:74:BB:0C:5E:BA:7A:FE:02:54:EF:7B:A0:C6:95:C6:09:80:70:96 - DoD Root CA 2
# 6C:8A:94:A2:77:B1:80:72:1D:81:7A:16:AA:F2:DC:CE:66:EE:45:C0 - DoD Root CA 3
# BD:C1:B9:6B:4D:F4:1D:EC:30:90:BF:62:73:C0:84:33:F2:71:24:85 - DoD Root CA 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this DoD Root CA 3?

Copy link
Member

Choose a reason for hiding this comment

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

I put these in here to be helpful and then I got them all wrong. I just pushed a commit to fix this comment.

@solipet solipet marked this pull request as ready for review June 18, 2020 15:20
@solipet solipet merged commit e125896 into master Jun 18, 2020
@solipet solipet deleted the dprice-lg-3043-map-bridge-certs branch June 18, 2020 17:55
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

3 participants