-
Notifications
You must be signed in to change notification settings - Fork 63
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 dedicated structures for PKCS12 and JKS stores #253
Conversation
Hi @arsenalzp. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arsenalzp Please try to avoid closing PRs and opening new ones. It makes it harder to track work on this new feature.
I still think the API additions need a bit more work, and again I think this is the most important part. Making a field with a default required does not make sense to me. Though a password IS required, it has a default. So it is not required from the user's point of view. I've put up some suggestions, but I am not sure about the min/max length requirements. It is not easy to find any good specification of these legacy formats.
Hello colleagues, |
Please rebase the PR and ensure you have run Let me know if you need help rebasing. It is a skill you should master, but it always feels a bit uncomfortable the first couple of times. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few suggestions. I wonder if we should not use the default passwords in tests. Ideally, the defaults should be just be present in the API spec.
All remarks were fixed, branch was re-based as well. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for this. Almost LGTM from me. Are we able to add some tests using this new feature? It seems like all tests use the default passwords.
Hello, |
I spent the whole evening with troubleshooting PKCS12 test case, and I've just found that PKCS12 encoders are not deterministic:
|
Yes, both JKS and PKCS12 adds some salt making them non-deterministic. I think the tests must be created with that in mind. What's wrong with the existing test approach? trust-manager/pkg/bundle/sync_test.go Lines 1541 to 1573 in 01bd331
|
Thank you for a hint! |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @arsenalzp! Did a quick review now. Just one suggestion. I would like to take a closer look and another review from a maintainer before we merge.
Fixes #199
/approve
/hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one small typo
@arsenalzp Maybe you can update the PR title and description? It seems like I am not allowed to do it. I suggest;
|
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a few minor bits from me - thank you for working on this! ❤️
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
/hold in case someone else wants to review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of suggestions to API docs improvements, but
/lgtm
Fixes were implemented. |
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> remove invalid comments in validation tests Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> add addtitional validation options for JKS and PKCS12 stores Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> fix remarks Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> tests for PKCS12 and JKS with password and change encoder to LegacyRC2 Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> improve sync tests for arbitrary password Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> fix typos, inrease MaxLength to 128 symbols Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> fix comments in Bundle types Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> fix tests of arbitrary password feature Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks a lot for finishing this @arsenalzp!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: erikgb, inteon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
This PR fixes #199 .
As was proposed by @erikgb, two dedicated structures PKCS12 and JKS were added.
Could you please be so kind to review this PR?