Skip to content

Conversation

@lhoguin
Copy link
Contributor

@lhoguin lhoguin commented Apr 4, 2023

Since ct_helper removes erl_make_certs some tests needed to be updated to use public_key:pkix_test_data/1 instead.

Special care must be taken during review around the long chain test since we expect the trusted and untrusted certificates to be using the same chain (they are issued by the same intermediary certificate).

@lhoguin lhoguin requested a review from the-mikedavis April 4, 2023 10:45
@mergify mergify bot added the bazel label Apr 4, 2023
Since ct_helper removes erl_make_certs some tests needed
to be updated to use public_key:pkix_test_data/1 instead.
@essen essen force-pushed the lh-update-ct-helper branch from 924ce52 to 53c6d19 Compare April 4, 2023 11:01
@lhoguin lhoguin marked this pull request as ready for review April 4, 2023 11:19

TestDataTrusted = public_key:pkix_test_data(#{
root => [],
intermediaries => [[{key, KeyInter}]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at public_key:chain_opts() I think this should be intermediates. It looks like it's also expecting the dec key:

Suggested change
intermediaries => [[{key, KeyInter}]],
intermediates => [[{key, KeyInterDec}]],

This will also add a new cert to the cacerts list on L340

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, this explains a few things I didn't understand.

[CertInter, RootCA] = proplists:get_value(cacerts, TestDataTrusted),

TestDataUntrusted = public_key:pkix_test_data(#{
root => [#{cert => CertInter, key => KeyInter}],
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this is expecting a proplist or a map: https://github.com/erlang/otp/blob/e599897f91ca5f05b4d868c6776efad7e6c1ad8d/lib/public_key/src/pubkey_cert.erl#L505-L523. (pubkey_cert:root_cert/2 tries to get the key out of this with proplists:get/3.)

It also looks like this one expects the dec key:

Suggested change
root => [#{cert => CertInter, key => KeyInter}],
root => [{cert, CertInter}, {key, KeyInterDec}],

Or alternatively I think this should work:

Suggested change
root => [#{cert => CertInter, key => KeyInter}],
root => #{cert => CertInter, key => KeyInterDec},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The map is correct, just not fully documented. It's how you can pass the root certificate from pkix_test_root_cert. Interesting I didn't get a crash because of the list though.

@lhoguin
Copy link
Contributor Author

lhoguin commented Apr 4, 2023

Pushed a commit with the fixes. Noticed the root CA is returned twice in cacerts (first and last) and fixed that as well. We now get the correct intermediate certificate I think.

@lhoguin lhoguin requested a review from the-mikedavis April 4, 2023 15:18
Copy link
Collaborator

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for updating this!

@michaelklishin michaelklishin merged commit b26b371 into main Apr 4, 2023
@michaelklishin michaelklishin deleted the lh-update-ct-helper branch April 4, 2023 17:52
michaelklishin added a commit that referenced this pull request Apr 4, 2023
michaelklishin added a commit that referenced this pull request Apr 4, 2023
michaelklishin added a commit that referenced this pull request Apr 5, 2023
Update ct_helper (backport #7821) (backport #7837) (backport #7838)
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.

4 participants