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

Move PEMCertPool from CTFE package to x509util #903

Merged
merged 2 commits into from
Apr 11, 2022
Merged

Move PEMCertPool from CTFE package to x509util #903

merged 2 commits into from
Apr 11, 2022

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented Apr 7, 2022

Currently many things indirectly depend on the entire CTFE package
(including HTTP handlers and some flags) because they use the cert pool.
For example, the ctclient CLI tool imports loglist, which imports ctfe.
This is a large overkill, and brings irrelevant flags to the CLI tool.

This change moves the cert pool type to x509util, because it's a wrapper
around the pool provided in the x509 package.

Checklist

@pav-kv pav-kv requested review from pphaneuf and AlCutter April 7, 2022 18:09
Copy link
Member

@AlCutter AlCutter left a comment

Choose a reason for hiding this comment

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

Seems reasonable; PEMCertPool feels like it's an internal implementation detail anyway, but if we do decide it's a worthwhile tradeoff to break semver for this we should clearly call it out in the CHANGELOG

@pphaneuf
Copy link
Contributor

The whole of the "internals" of CTFE (such as the ctfe package) should probably all be moved into an internal directory, while we're breaking things, but that's something else.

Currently many things indirectly depend on the entire CTFE package
(including HTTP handlers and some flags) because they use the cert pool.
For example, the ctclient CLI tool imports loglist, which imports ctfe.
This is a large overkill, and brings irrelevant flags to the CLI tool.

This change moves the cert pool type to x509util, because it's a wrapper
around the x509 pool.
@pav-kv
Copy link
Contributor Author

pav-kv commented Apr 11, 2022

Updated changelog.

@pav-kv pav-kv merged commit a644325 into google:master Apr 11, 2022
@pav-kv pav-kv deleted the move_pem_cert_pool branch April 11, 2022 13:44
@pav-kv pav-kv mentioned this pull request May 4, 2022
2 tasks
pav-kv added a commit that referenced this pull request May 4, 2022
This change removes the dependency of x509util tests from ctfe/testonly
package introduced in #903.

There are 2 certificates copy-pasted between the two packages, but they don't
strictly need to match, and having them independent is a bigger advantage.
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