-
Notifications
You must be signed in to change notification settings - Fork 64
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
enable Request.Format #277
base: master
Are you sure you want to change the base?
Conversation
Hi @eyalle , We appreciate any contribution to our Open Source integrations. Could you add a corresponding test or tests for this implementation in https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits PD: we also noticed that you opened this PR with the exact same changes: https://github.com/Venafi/vcert/pull/276/files |
ce6d392
to
19e7412
Compare
Hi @luispresuelVenafi , Thank you for the comment & instructions! 🙏 I had some issues with signing the commits, but is should be sorted out now. Can you please take a second look to see it's compatible with your workflows?.. |
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.
Hi @eyalle,
We run this in our automated pipeline and the following is failing:
- From aruba cucumber:
Failing Scenarios:- cucumber features/enroll/enroll-deprecated-options.feature:29 # Scenario: ~ Service Generated CSR pickup later ID as param ~
Output:
- cucumber features/enroll/enroll-deprecated-options.feature:29 # Scenario: ~ Service Generated CSR pickup later ID as param ~
Scenario: ~ Service Generated CSR pickup later ID as param ~ # features/enroll/enroll-deprecated-options.feature:29
vcert enroll -tpp-url '***' -tpp-user '***' -tpp-password '***' -insecure -z '<zone>' -csr service -cn <zone>\***.vcert.example -no-pickup
When I enroll certificate using TPPdeprecated with -csr service -cn ****.vcert.example -no-pickup # features/step_definitions/actions.rb:12
Then it should post certificate request # features/step_definitions/my_steps.rb:17
vcert pickup -tpp-url '***' -tpp-user '***' -tpp-password '***' -insecure -pickup-id '<zone>/***.vcert.example' -key-password *** -timeout 59
And I retrieve the certificate from TPPdeprecated using the same Pickup ID with -key-password *** -timeout 59 # features/step_definitions/actions.rb:36
Then it should retrieve certificate # features/step_definitions/my_steps.rb:10
Then it should output encrypted private key # features/step_definitions/my_steps.rb:24
expected "PickupID=\"<zone>\\***.vcert.example\"\n-----B...trieved request for <zone>\\***.vcert.example" to string includes: "-----BEGIN ENCRYPTED PRIVATE KEY-----"
Diff:
@@ -1,116 +1,231 @@
------BEGIN ENCRYPTED PRIVATE KEY-----
+PickupID="<zone>\****-param.vcert.example"
+-----BEGIN CERTIFICATE-----
+MIIH2jCCBcKgAwIBAgITbQCwGPQnBw1wWUtk0gAAALAY9DANBgkqhkiG9w0BAQsF
+...
+am0ey+Y7hW9PQxuPSXHzDi/bW/R+bSLhImHTtGxWapzxou+KtdBOb8s7dhsRBA==
+-----END CERTIFICATE-----
+-----BEGIN RSA PRIVATE KEY-----
+Proc-Type: 4,ENCRYPTED
+DEK-Info: DES-EDE3-CBC,B09C40F7AC2209DC
+
+1bJz9LbKIDdV3+vaR1vVUJuLjbX59sPq4SXKPKz/hTUKvTDBYSkeVODVYfl0Fa6r
+...
+qbkc+/DjP3TaWB6dllq3XWt/Qn1P/Dz+cggPCqH49gRONk0nOe34uw==
+-----END RSA PRIVATE KEY-----
+-----BEGIN CERTIFICATE-----
+MIIFpjCCA46gAwIBAgIQPY6aY41C6JxH4BxIUMuftTANBgkqhkiG9w0BAQsFADBb
+...
+QXe+VMF9FXaRqDI/cCNsBnR++USinZvwGY6SecfDtHA7x65yJol7Y8YtURNfyDfg
+yVzOWlPcu2gJaw==
+-----END CERTIFICATE-----
+vCert: 2023/02/13 22:27:06 Warning: User\Password authentication is deprecated, please use access token instead.
+vCert: 2023/02/13 22:27:06 Successfully connected to Trust Protection Platform
+vCert: 2023/02/13 22:27:06 Successfully read zone configuration for ****
+vCert: 2023/02/13 22:27:06 Successfully created request for ****.vcert.example
+vCert: 2023/02/13 22:27:06 Successfully posted request for ***.vcert.example, will pick up by <zone>\***.vcert.example
+vCert: 2023/02/13 22:27:07 Successfully connected to Trust Protection Platform
+vCert: 2023/02/13 22:27:07 Issuance of certificate is pending...
+vCert: 2023/02/13 22:27:12 Successfully retrieved request for <zone>\***.vcert.example
(RSpec::Expectations::ExpectationNotMetError)
./features/step_definitions/my_steps.rb:26:in `/^it should( not)? output( encrypted)? private key$/'
features/enroll/enroll-deprecated-options.feature:34:in `it should output encrypted private key'
# cls
# title ~ Service Generated CSR pickup later ID in file~
# VCert enroll -tpp-url %TPP_URL% -tpp-user %TPP_USER% -tpp-password %TPP_PASS% -z "%POLICY%" -csr service -cn ***.vcert.example -no-pickup -pickup-id-file pickup_id.txt
# timeout /t 15 /nobreak
# echo.
# VCert pickup -tpp-url %TPP_URL% -tpp-user %TPP_USER% -tpp-password %TPP_PASS% -pickup-id-file pickup_id.txt -key-password %KEY_PASS%
# if ERRORLEVEL 1 goto :DONE
# timeout /t 10
# cls
# title ~ User Provided CSR with RSA key ~
# VCert gencsr -cn ***.vcert.example -key-type rsa -key-size 4096 -key-file ***-rsa.key -csr-file ***-rsa.req -no-prompt
# echo.
# VCert enroll -tpp-url %TPP_URL% -tpp-user %TPP_USER% -tpp-password %TPP_PASS% -z "%POLICY%" -csr file:***-rsa.req
# if ERRORLEVEL 1 goto :DONE
# timeout /t 10
- From
connector_test.go
:- Test_GetCertificateListFull
Output:
- Test_GetCertificateListFull
=== RUN Test_GetCertificateListFull
vCert: 2023/02/13 23:05:48 Getting guid for object DN <zone>.****.vfidev.com - 118
connector_test.go:1737: list should be longer
--- FAIL: Test_GetCertificateListFull (11.53s)
=== RUN TestEnrollWithLocation
Could you verify that this tests are working in your end and fix them as needed?
Hi @luispresuelVenafi still working on it, having some issues working against our company's Venafi. |
Hi @luispresuelVenafi apparently, I've mixed up the order of setting the default req.Format |
Hi @eyalle Sorry for late response. Sure, we will look into it 😄 |
Thank you @luispresuelVenafi ! 🙏 |
@eyalle everything looking good. Will ask tomorrow about merging |
Thank you! @luispresuelVenafi |
@luispresuelVenafi any updates on merging this? 😇 |
Hello there @eyalle, There is something not clear to me with this Is your I want to understand this as to not break the current functionality of the cli tool. The thing is I feel we may need to change the cli tool as well with this change |
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.
Check comment above
@tr1ck3r could you look at this PR? Are there any possible side-effects of adding the format variable to the request object? |
Hi @rvelaVenafi ! The Does that answer your question? |
Hello there @eyalle, it makes sense. Just a couple of observations. Given that now the format is open to user input, it would be better to have it narrowed to a fixed set of values. From 22.4 TPP docs, the format attribute can take the following values:
So I would recommend changing the CertificateRequest.Format attribute to an enum with the previous values as the enum options:
Then a String function to return the string value for each enum:
The last cases in the switch ensure that when no value is set, the default is Base 64 |
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.
Blocking while checking how this works on our pipeline
HI there @eyalle , With this latest changes, now our pipeline is failing this test within our cucumber tests (against master, runs fine):
Could you verify this runs fine on your end? |
Hi @luispresuelVenafi |
pkg/venafi/tpp/connector.go
Outdated
@@ -1202,6 +1202,14 @@ func (c *Connector) RetrieveCertificate(req *certificate.Request) (certificates | |||
includeChain := req.ChainOption != certificate.ChainOptionIgnore | |||
rootFirstOrder := includeChain && req.ChainOption == certificate.ChainOptionRootFirst | |||
|
|||
// if Request doesn't contain a Format, use defaults | |||
if req.Format.String() == "" { |
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.
This comparison must be done against CertFormatNotSet, not an string.
if req.Format == CertFormatNotSet {
...
}
pkg/venafi/tpp/connector.go
Outdated
} | ||
|
||
fmt.Println("\n\n", certReq) |
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.
Either remove this print or put it behind a debug flag.
pkg/venafi/tpp/connector.go
Outdated
@@ -1202,6 +1202,11 @@ func (c *Connector) RetrieveCertificate(req *certificate.Request) (certificates | |||
includeChain := req.ChainOption != certificate.ChainOptionIgnore | |||
rootFirstOrder := includeChain && req.ChainOption == certificate.ChainOptionRootFirst | |||
|
|||
// if Request doesn't contain a Format, use defaults |
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.
I think this condition should go back to where it originally was. Below line 1232 if req.CsrOrigin == certificate.ServiceGeneratedCSR || req.FetchPrivateKey {
Otherwise we are always setting certificate format to Base64 PKCS8
when the key is RSA
.
The default value is already provided by FormatNotSet enum var, so no conditional is necessary before setting the fomrat.
PKCS8
must only happen when the key type is RSA
and the csrOrigin is Service
, or the user chooses to use it.
…ng the Request Format - pkg/venafi/tpp/connector: set a default to the request.Format in `RetrieveCertificate()`, unless a Format is explicitly passed
…ormat to determine if the default is set (tests with passing Format already exist)
- update logic of `RetrieveCertificate()` to accommodate eNum changes for `Request.Format`
c6f37f7
to
4898917
Compare
@rvelaVenafi @luispresuelVenafi |
Hi @eyalle , I see you only added test for mock. Since we are introducing a bigger change here by allowing this new attribute to specify the form with different Enum types, could you also add functional unit tests (not only mock) about it? In |
Hi @luispresuelVenafi ! |
While working on a project that utilizes this library, I had a need for specifying the Format of the certificate Request that's being used for invoking the RetrieveCertificate() function.
This PR allows passing the expected Format in a
certificateRetrieveRequest()
function invoke.The existing default setting was preserved, and only in case it's explicitly passed - the value will be used.