feat: add pkiSign function to use vault pki sign api endpoint #1905
Conversation
n-marton
force-pushed
the
feat/add-pkisign
branch
from
daa303a to
fe6b688
Compare
n-marton
force-pushed
the
feat/add-pkisign
branch
2 times, most recently
from
bc92044 to
395376a
Compare
|
hey guys, any chance to have a look on that? |
Signed-off-by: n-marton <[email protected]>
n-marton
force-pushed
the
feat/add-pkisign
branch
from
395376a to
3dd7245
Compare
|
Hello team, could please someone have a look? It will be nice to have it soon. Thanks in advance! |
Co-authored-by: Amir Aslamov <[email protected]>
Co-authored-by: Amir Aslamov <[email protected]>
Co-authored-by: Amir Aslamov <[email protected]>
Co-authored-by: Amir Aslamov <[email protected]>
Co-authored-by: Amir Aslamov <[email protected]>
Co-authored-by: Amir Aslamov <[email protected]>
|
Hi @n-marton , thanks a lot for opening the PR and your contribution. Your changes look great! I left some feedback. Also, we need unit tests added in here to ensure your new functions are covered in unit tests (to test they work in different use cases and edge cases and in case someone changes it in the future, unit tests catch errors): https://github.com/hashicorp/consul-template/blob/main/template/funcs_test.go |
Co-authored-by: Amir Aslamov <[email protected]>
hey @aslamovamir thanks for getting back on this MR, I haven't wrote a test there because the cert issue func also did not have one, in fact that part is kind lacking heavily all together, however I am happy to write one for my new func, IF that is the only one thing we miss to merge this feature |
Hi @n-marton, yup I see - I honestly am not part of the codeowners for this repo or never contributed to it so did not know other functions also lack unit tests here. But would be great to have it written for yours if thats okay. All the rest looks good to me in your PR and can approve it once we have unit test coverage. |
victorr
left a comment
victorr
left a comment
There was a problem hiding this comment.
This is a wonderful PR, thank you so much for your contribution, it should prove very useful.
I have added some comments for your consideration. The most important one is about supporting alt_names. Please also consider addressing the other comments, or at least mentioning in the documentation the the defaults you have used for the signature algorithms.
I am not one of the code owners for this project, but we should have someone from the right team to review the PR soon.
| } | ||
| // if we passed use_csr_sans, that means serverside will expect the subject alternate names from the csr | ||
| // so besides adding that param to the csr template, we also remove it from the map we pass later | ||
| if useCSRSans { |
There was a problem hiding this comment.
The role's use_csr_sans overrides not only parameters uri_sans and ip_sans, but also alt_names.
It would be wonderful if you could add the names inalt_names to the CSR. The names have to be sorted into email addresses and DNS names, you can see how Vault does that here:
| return nil, err | ||
| } | ||
| privateKey = key | ||
| csrTemplate.SignatureAlgorithm = x509.SHA512WithRSA |
There was a problem hiding this comment.
What about looking for a signature_bits parameter to set the signature algorithm? Please consider it, as well as use_pss parameter for the RSA case.
| TTL, which can put a high load on the Vault servers. | ||
|
|
||
| The templating behavior is the same as in `pkiCert`, with a few special attributes. | ||
| You also need to pass `key_type=rsa|ec|ed25519` in alignment with your |
5 participants
This pull requests intends to add a function to leverage the
signapi endpoint in vault pki.This can be really useful if one plans to use a large number of certificates with low TTL, where the usage of the
issueendpoint can cause high load on the vault servers. By using the sign endpoint one can shift the heaviest part of the problem (the private key generation), from server side to client side.