-
Notifications
You must be signed in to change notification settings - Fork 17
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
SECURESIGN-1460 | Ensure that TSA component does not create duplicated resources #675
SECURESIGN-1460 | Ensure that TSA component does not create duplicated resources #675
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JasonPowr 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 |
e3b2a26
to
7eef591
Compare
7074c31
to
6cd35b9
Compare
91ba0c6
to
455f8bc
Compare
8911e07
to
6052c53
Compare
as I am part of the development @osmman can you re-review? |
/test tas-operator-e2e |
I think that rootCA, intermediateCA, leafCA should not be created if signer uses key from is KMS or Tink service. These should be using certificate chain stored in Here PoC which splits management of signer and cert chains into different actions. |
@osmman Does it create rootCA, intermediateCA, leafCA keys when other signer types are specified ? Originally it wasn't supposed to, here:
and we will only create a cert chain if one is not specified secure-sign-operator/internal/controller/tsa/actions/generate_signer.go Lines 329 to 343 in 6052c53
|
I think that the action will fail if KMS or Tink resource and cert chain ref is empty/configured to autocreate certs. It will try to resolve certificate chain
which need RootPrivateKey. That value do not exists because it is resolved only for File based signer. |
6052c53
to
e0fc6bd
Compare
/test tas-operator-e2e |
9ed68a2
to
cb3f8a8
Compare
cb3f8a8
to
952efe0
Compare
/lgtm |
@bouskaJ @osmman Just looking for some feed back on this approach to ensuring only one resources is created at a time, if its acceptable Ill apply it to other TSA resources, needs some more testing