Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

eyalle
Copy link

@eyalle eyalle commented Feb 9, 2023

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.

@luispresuelVenafi
Copy link
Contributor

Hi @eyalle ,

We appreciate any contribution to our Open Source integrations. Could you add a corresponding test or tests for this implementation in /pkg/venafi/tpp/connector_test.go ? Could you also sign your commit? It is a requirement in order to accept changes in our repositories:

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
if you feel that you made a mistake by closing it, please, re-open it instead of creating a new PR; for now, we will follow this PR, but just mentioning it so you take it into account for any next contribution 😉

@eyalle eyalle force-pushed the eyalle/support_req_format branch 3 times, most recently from ce6d392 to 19e7412 Compare February 13, 2023 13:18
@eyalle
Copy link
Author

eyalle commented Feb 13, 2023

Hi @luispresuelVenafi ,

Thank you for the comment & instructions! 🙏

I had some issues with signing the commits, but is should be sorted out now.
I've also added a test under the file requested.

Can you please take a second look to see it's compatible with your workflows?..

Copy link
Contributor

@luispresuelVenafi luispresuelVenafi left a 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:
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:
=== 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?

@eyalle
Copy link
Author

eyalle commented Feb 26, 2023

Hi @luispresuelVenafi
Thank you for this!

still working on it, having some issues working against our company's Venafi.
Will update once I overcome these 🙏

@eyalle
Copy link
Author

eyalle commented Feb 28, 2023

Hi @luispresuelVenafi
took some time to setup my environment and reproduce the errors you gracefully attached (thanks again!)

apparently, I've mixed up the order of setting the default req.Format
both failing tests are now showing as good.
Any chance you can check your CI and let me know if it's ok now?

@luispresuelVenafi
Copy link
Contributor

Hi @eyalle

Sorry for late response. Sure, we will look into it 😄

@eyalle
Copy link
Author

eyalle commented Mar 9, 2023

Hi @eyalle

Sorry for late response. Sure, we will look into it 😄

Thank you @luispresuelVenafi ! 🙏

@luispresuelVenafi
Copy link
Contributor

@eyalle everything looking good. Will ask tomorrow about merging

@eyalle
Copy link
Author

eyalle commented Mar 15, 2023

@eyalle everything looking good. Will ask tomorrow about merging

Thank you! @luispresuelVenafi
can't wait to use the new version :)

@eyalle
Copy link
Author

eyalle commented Mar 19, 2023

@luispresuelVenafi any updates on merging this? 😇

@rvelaVenafi
Copy link
Contributor

rvelaVenafi commented Mar 21, 2023

Hello there @eyalle,

There is something not clear to me with this Format variable. The CLI tool (vcert) already has a Format flag which values can be: legacy-pem, pkcs12 or jks.

Is your Format variable somehow related to this variable?
What are the possible values for the Request.Format variable in your PR?

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

Copy link
Contributor

@rvelaVenafi rvelaVenafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check comment above

@rvelaVenafi
Copy link
Contributor

@tr1ck3r could you look at this PR? Are there any possible side-effects of adding the format variable to the request object?

@eyalle
Copy link
Author

eyalle commented Mar 22, 2023

Hello there @eyalle,

There is something not clear to me with this Format variable. The CLI tool (vcert) already has a Format flag which values can be: legacy-pem, pkcs12 or jks.

Is your Format variable somehow related to this variable? What are the possible values for the Request.Format variable in your PR?

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

Hi @rvelaVenafi !

The Format I added was for Certificate.Request
We're using it to pass base64; we've debugged this locally as the certificates retrieved by the CLI didn't match what we're using in our Venafi.
After adding this Format, we got our certs successfully.
We kept the logic as is, just added the possibility of specifying the Format.
We needed a way to have the RSA KeyType, and base64 as the Format 🙏

Does that answer your question?

@eyalle eyalle removed the request for review from tr1ck3r March 22, 2023 11:13
@eyalle eyalle requested a review from rvelaVenafi March 22, 2023 11:13
@rvelaVenafi
Copy link
Contributor

rvelaVenafi commented Mar 22, 2023

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:

  • Base64: Show the certificate in the traditional PEM format. The mime-type is application/x-pem-file.
  • Base64 (PKCS #8): Show the certificate in the PEM format and include the PKCS#8 encoded private key. The mime-type is application/x-pem-file.
  • DER: Show the raw certificate. Use IncludePrivateKey and set the password. The mime-type is application/x-x509-ca-cert.
  • JKS: Show the certificate in the Java Keystore (JKS) format. The mime-type is application/octet-stream.
  • PKCS #7: Show the certificate with optional chain in the PKCS#12 format. The mime-type is application/x-pkcs7-certificates.
  • PKCS #12: Show the certificate in the PKCS#12 format. Use IncludePrivateKey and set the password. The mime-type is application/x-pkcs12.

So I would recommend changing the CertificateRequest.Format attribute to an enum with the previous values as the enum options:

type CertificateFormat int
const(
    CertFormatNotSet CertificateFormat = iota
    CertFormatBase64
    CertFormatBase64PKCS8
    CertFormatDER
    CertFormatJKS
    CertFormatPKCS7
    CertFormatPKCS12
)
type CertificateRequest struct {
    ...
    Format CertificateFormat
}

Then a String function to return the string value for each enum:

func (f CertificateFormat) String() {
    switch f:  {
    case CertFormatBase64PKCS8:
        return "Base64 (PKCS #8)"
    ...
    ...
    ...
    case CertFormatBase64:
        fallthrough
    case CertFormatNotSet:
        fallthrough
    default:
        return "Base 64"
    }
}

The last cases in the switch ensure that when no value is set, the default is Base 64

Copy link
Contributor

@luispresuelVenafi luispresuelVenafi left a 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

@luispresuelVenafi
Copy link
Contributor

HI there @eyalle ,

With this latest changes, now our pipeline is failing this test within our cucumber tests (against master, runs fine):

Scenario: ~ Service Generated CSR with RSA key ~                                                                                                                        # features/enroll/enroll-deprecated-options.feature:14
09:39:31  vcert enroll  -tpp-url 'https://****/vedsdk' -tpp-user '<user>' -tpp-password '****' -insecure -z 'Open Source\vcert'  -csr service -key-type rsa -key-size 4096 -cn service-gen-rsa.vcert.example -format json -key-password **** 
09:39:37      When I enroll a certificate in TPPdeprecated with -csr service -key-type rsa -key-size 4096 -cn service-gen-rsa.vcert.example -format json -key-password **** # features/step_definitions/actions.rb:12
09:39:37      Then it should retrieve certificate                                                                                                                                   # features/step_definitions/my_steps.rb:10
09:39:37      Then I get JSON response                                                                                                                                              # features/step_definitions/openssl.rb:4
09:39:37        859: unexpected token at '{\Path\to\service-gen-rsa.vcert.example base64  false true  false}
09:39:37        {"Certificate":"-----BEGIN CERTIFICATE-----\nMIIIvDCCB ... YXO9+Rf2NDufTbmZUs=\n-----END CERTIFICATE-----\n","PrivateKey":"-----BEGIN RSA PRIVATE KEY-----\r\nProc-Type: 4,ENCRYPTED\r\nDEK-Info: DES-EDE3-CBC,E63886DE7FCC224A\r\n\r\nzlOpLBUeMdCXcjrfgCE3txE/7Ja6x ... 15sWtmwporzmneiD/W8qgtem\r\n-----END RSA PRIVATE KEY-----\r\n","Chain":["-----BEGIN CERTIFICATE-----\nMIIFpj ... yJol7Y8YtURNfyDfg\nyVzOWlPcu2gJaw==\n-----END CERTIFICATE-----\n"],"PickupId":"\\Path\\to\\service-gen-rsa.vcert.example"}' (JSON::ParserError)
09:39:37        ./features/step_definitions/openssl.rb:6:in `/^I get JSON response$/'
09:39:37        features/enroll/enroll-deprecated-options.feature:17:in `I get JSON response'
09:39:37      And that certificate should contain "Public-Key: (4096 bit)"                                                                                                          # features/step_definitions/openssl.rb:53
09:39:37  # cls
09:39:37  # title ~ Service Generated CSR pickup later ID as param ~
09:39:37  # for /f "tokens=2 delims==" %%i in ( 'VCert enroll -tpp-url %TPP_URL% -tpp-user %TPP_USER% -tpp-password %TPP_PASS% -z "%POLICY%" -csr service -cn service-gen-pickup-id-as-param.vcert.example -no-pickup 2^>^&1 ^| find "PickupID="' ) do set PICKUP_ID=%%i
09:39:37  # echo PickupID=%PICKUP_ID%
09:39:37  # timeout /t 15 /nobreak
09:39:37  # echo.
09:39:37  # VCert pickup -tpp-url %TPP_URL% -tpp-user %TPP_USER% -tpp-password %TPP_PASS% -pickup-id %PICKUP_ID% -key-password %KEY_PASS%
09:39:37  # if ERRORLEVEL 1 goto :DONE
09:39:37  # timeout /t 10
09:39:37  # cls
09:39:37  # title ~ Service Generated CSR pickup later ID in file~
09:39:37  # VCert enroll -tpp-url %TPP_URL% -tpp-user %TPP_USER% -tpp-password %TPP_PASS% -z "%POLICY%" -csr service -cn service-gen-pickup-id-in-file.vcert.example -no-pickup -pickup-id-file pickup_id.txt
09:39:37  # timeout /t 15 /nobreak
09:39:37  # echo.
09:39:37  # VCert pickup -tpp-url %TPP_URL% -tpp-user %TPP_USER% -tpp-password %TPP_PASS% -pickup-id-file pickup_id.txt -key-password %KEY_PASS%
09:39:37  # if ERRORLEVEL 1 goto :DONE
09:39:37  # timeout /t 10
09:39:37  # cls
09:39:37  # title ~ User Provided CSR with RSA key ~
09:39:37  # VCert gencsr -cn user-provided-rsa.vcert.example -key-type rsa -key-size 4096 -key-file user-provided-rsa.key -csr-file user-provided-rsa.req -no-prompt
09:39:37  # echo.
09:39:37  # VCert enroll -tpp-url %TPP_URL% -tpp-user %TPP_USER% -tpp-password %TPP_PASS% -z "%POLICY%" -csr file:user-provided-rsa.req
09:39:37  # if ERRORLEVEL 1 goto :DONE
09:39:37  # timeout /t 10
09:39:37  
09:39:37  Failing Scenarios:
09:39:37  cucumber features/enroll/enroll-deprecated-options.feature:14 # Scenario: ~ Service Generated CSR with RSA key ~

Could you verify this runs fine on your end?

@eyalle
Copy link
Author

eyalle commented Mar 26, 2023

Hi @luispresuelVenafi
Thank you for checking!
I did see this error; I removed an accidental print that messed up with the Cucumber test results.
May I trouble you please to check again?.. 🙏
While I do see errors still, I think it may be related to our Venafi configuration\policies which I don't have access to

@@ -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() == "" {
Copy link
Contributor

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 {
    ...
}

}

fmt.Println("\n\n", certReq)
Copy link
Contributor

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.

@@ -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
Copy link
Contributor

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`
@eyalle
Copy link
Author

eyalle commented Mar 27, 2023

@rvelaVenafi @luispresuelVenafi
I've moved the check after the CSR handling 🙏

@eyalle eyalle requested review from rvelaVenafi and luispresuelVenafi and removed request for rvelaVenafi and luispresuelVenafi March 27, 2023 15:42
@luispresuelVenafi
Copy link
Contributor

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 /pkg/venafi/tpp/connector_test.go , you could add, for example, a test called TestRetrieveCertBase64 that specifies CertFormatBase64 and do an assert to verify that response from TPP is indeed in Base64 format.

@eyalle
Copy link
Author

eyalle commented May 11, 2023

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 /pkg/venafi/tpp/connector_test.go , you could add, for example, a test called TestRetrieveCertBase64 that specifies CertFormatBase64 and do an assert to verify that response from TPP is indeed in Base64 format.

Hi @luispresuelVenafi !
Sorry for the late response 🙏
I'm not entirely sure how to set up the unit test, is there a chance we can set up a quick Zoom for a few minutes to tackle this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants