search

cert-manager / aws-privateca-issuer

Addon for cert-manager that issues certificates using AWS ACM PCA.
Apache License 2.0
192 stars 81 forks source link

use sha256 sum for idempotency token #340

Closed mritunjaysharma394 closed 1 month ago

mritunjaysharma394 commented 1 month ago

As of now we use a bit less secure md5 for idempotency token, I think we should prefer to use more secure sha256sum for it and this PR aims to do that, thanks!

divyansh-gupta commented 1 month ago

Hey @mritunjaysharma394 - Thanks for the PR! So we can better understand, what exactly are the security benefits of using Sha256 over Md5 for an idempotency token? Idempotency tokens are not secure or sensitive information, so its not immediately clear to me.

Thanks!

mritunjaysharma394 commented 1 month ago

Hey @divyansh-gupta , thanks for your reply and review! While, idempotency tokens are not secure or sensitive information, using MD5 in this may make it non-FIPS compliant by most FIPS verification check tests due to it. Thus when this is run in environments which are strict FIPS compliant, it may fail to run or show unwarranted behaviours. Using sha256 may save us from that. Thanks!

ndbhat commented 1 month ago

Hi @mritunjaysharma394, the tests are failing because the idempotency token has a max length of 36 https://docs.aws.amazon.com/privateca/latest/APIReference/API_IssueCertificate.html#privateca-IssueCertificate-request-IdempotencyToken

operation error ACM PCA: IssueCertificate, https response error StatusCode: 400, RequestID: 807f511a-b6ef-4432-999d-f6d41f7e4c8b, api error ValidationException: 1 validation error detected: Value '6a2bccd1c8ba53886ab3f8a2b235fd416cdf508aa351d0ebd48b3e5740e8b96c' at 'idempotencyToken' failed to satisfy constraint: Member must have length less than or equal to 36

mritunjaysharma394 commented 1 month ago

Thanks so much @ndbhat for the review, apologies for not knowing that, I attempted a fix, let me know if it will work, thanks so much again!

ARichman555 commented 1 month ago

Looks like testing is passing now, would you mind squashing these commits into one first? We'll get it merged after that

mritunjaysharma394 commented 1 month ago

Thanks @ARichman555 , sure I can do that and will do it right now

mritunjaysharma394 commented 1 month ago

Hi @ARichman555 @ndbhat, squashed the commits, lmk if it looks good now, thanks!

cert-manager-prow[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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: - **[OWNERS](https://github.com/cert-manager/aws-privateca-issuer/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment