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

feature: implement policy store #2

Merged
merged 1 commit into from May 3, 2021
Merged

feature: implement policy store #2

merged 1 commit into from May 3, 2021

Conversation

ereslibre
Copy link
Member

No description provided.

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Everything looks good, on top of the comment I left I have two observations:

  • This changes from the native-tls to rusttls. This is cool from a certain POV, I'm thinking about a statically linked kwctl binary. However, this crate will be used also inside of the policy-server, which is stuck with the native-tls crate. I don't think this will cause issues, do you?
  • We really need some tests for the methods that you introduced, like the ones that build all the paths for the local store. I'm not saying we have to go "crazy" and provide e2e tests that download things from real registries/http/https servers, but there are definitely some low hanging fruits from the testing POV that we must grab

src/https.rs Outdated Show resolved Hide resolved
@ereslibre
Copy link
Member Author

ereslibre commented May 3, 2021

This changes from the native-tls to rusttls. This is cool from a certain POV, I'm thinking about a statically linked kwctl binary. However, this crate will be used also inside of the policy-server, which is stuck with the native-tls crate. I don't think this will cause issues, do you?

I don't think so, in principle. The policy-server will do the TLS termination of the requests made by the apiserver, whereas when fetching policies we'll use rustls. They have different CA default stores -- meaning, rustls has its own main CA store, while native-tls uses the system one.

I think we can add a task for checking if this is the best combination and possible hazards later. Meaning: for doing the TLS termination we don't care that much about root CA certificates (and we are using the system certs by using native-tls at this time), but we care a lot about the CA store of the policy-fetcher (using rustls). How secure is to rely on rustls for this? Before bumping dependencies in this case we should check that the store included by rustls only contains well-known CA's, and only those well known, nothing more.

We really need some tests for the methods that you introduced, like the ones that build all the paths for the local store. I'm not saying we have to go "crazy" and provide e2e tests that download things from real registries/http/https servers, but there are definitely some low hanging fruits from the testing POV that we must grab

👍

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

LGTM, please... start working on the test suite

@ereslibre ereslibre merged commit c22b5fa into main May 3, 2021
@ereslibre ereslibre deleted the refactor branch May 3, 2021 15:07
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

2 participants