Contact Emails: RequestCertificate can now be given contact emails

[maelvls]

Some users need the ability to individually configure contacts for each certificate in TPP.

In some organizations using TPP, such as in https://github.com/Venafi/ansible-collection-venafi/issues/24#issuecomment-1078444360, it isn't possible (or practical) to create a separate policy folder per team, and many teams end up sharing the same policy folder. This is often the case in Kubernetes: platform teams prefer requesting a single policy folder and a single cert-manager ClusterIssuer for all the teams.

The problem that arises is that all the certificates end up with the same "contact", usually the platform team's contact. This is a problem because each team is responsible for their own certificates life cycle and, therefore, need the contact persons be assigned to the certificates directly.

In this PR, I propose to add a way to set "contacts" when requesting certificates through the Go SDK.

[!WARNING]

Requesting a certificate with the Contact field doesn't mean that the person who will receive the notification will be able to act on the certificate or access said certificate. The access is configured at the policy folder level, and if the user doesn't permissions on that folder, they will not be able to see the certificate and remediate the problem through the TPP UI.

Considerations:

1. When a contact matches multiple TPP identities, the first identity found is picked.
2. The scope configuration is required when using the new Contact field. Since Contacts works by searching the emails in the same LDAP or AD as the user attached to the token, you must check that you are using a user in that same identity provider. Contacts doesn't work with the local TPP identities. Using Contacts requires adding mail to the list of fields searched when performing a user search, which can be configured in the Venafi Configuration Console by RDP'ing into the TPP VM. This configuration cannot be performed directly in the TPP UI.
3. The user used to get a token in use by VCert must belong to an identity provider if you want to use an email or a UPN, and configuration in the identity provider is required.
4. The end-to-end test I wrote is disabled. The instance https://supertreat.venqa.venafi.com/ only has local users and this feature requires TPP to be configured with LDAP or AD.
5. Regarding the performance impact, we expect to have to do between one and two calls to POST /identity/browse anytime a certificate is requested.

Links (private to Venafi):

• Use-case on Confluence
• Jira ticket

To be considered:

[marcos-albornoz]

Hi Mael, I'm sharing here a patch with changes that are based in the changes here, the changes includes:

• The tpp.Connector.resolveContacts() method was refactored so it can be user not only for SetPolicy purpose but also for Request Certificate.
• The structs related to Identity were moved from policy package to tpp package.
• The tpp.prepareRequest() function was converted in a tpp.Connector method and it also was added the management to resolve and set the Contacts to the TPP Certificate Request.

Please review the changes and if you are ok with them you can include them into your changes. I have not tested them yet but them should work.

To apply the patch to your local branch only need to checkout this branch and then use the git apply command in the root folder of your project as follow: git apply contacts.patch

[maelvls]

@marcos-albornoz I turned your patch into a commit and pushed it so that we can discuss it in the PR.

About resolveContacts being refactored so that it can be used for emails, there is still a problem with getIdentities: getIdentities is unable to search for emails due to the check on the username that is performed by getIdentityMatching.

[maelvls]

@marcos-albornoz I had to undo some of the changes due to getIdentity only working for usernames.

If only we could know why getIdentityMatching is checking that the queried filter matches the identity's name, we could then combine resolveContacts and resolveCertificateRequestContacts... but until then, the only possible way is to keep these funcs separate.

To clarify, with LDAP or AD users, the returned identity does contain the Name field.

For example, let's imagine I call

getIdentity("mael.valais@venafi.com")

The call to /identities/browse will return the following:

{
  "Identities": [
    {
      "FullName": "CN=mvalais,CN=Users,DC=domain,DC=local",
      "Name": "mvalais",
      "Prefix": "LDAP+LDAP Microsoft AD localhost",
      "PrefixedName": "LDAP+LDAP Microsoft AD localhost:mvalais",
      "PrefixedUniversal": "LDAP+LDAP Microsoft AD localhost:0121ee0e-db7a-4744-96f1-09925259e9f4",
      "Type": 1,
      "Universal": "0121ee0e-db7a-4744-96f1-09925259e9f4"
    }
  ]
}

The problem is that getIdentityMatching is then comparing mvalais to mael.valais@venafi.com, which obviously are different.

I have also added an end-to-end test. I had to add t.Skip to it because https://supertreat.venqa.venafi.com/ doesn't have an LDAP or AD identity connector configured.

[marcos-albornoz]

Hi Mael, please review here a little update of the previous patch which I though is solving the issue in the getIdentity() and getIdentityMatching() methods.

[maelvls]

I now understand why getIdentity needs to check that the queried username is equal to the identity's username, and I reproduced it with a live TPP server! I described my findings in https://github.com/Venafi/vcert/pull/221#issuecomment-1911747389.

Now that I know why it is doing that, I was able to write unit tests for getIdentity in #420. Can you review that? My goal is to add tests so that we feel feel more confident about changing getIdentity.

[maelvls]

Thanks for the patch! I applied it to the PR (c7040c30008c4efea1383c6b90f07367aac165a9).

I propose to simplify the logic in getIdentityMatching by spelling out each case:

switch {
case len(resp.Identities) == 0:
    return nil, fmt.Errorf("no identity found for '%s'", filter)
case len(resp.Identities) == 1:
    return &resp.Identities[0], nil
case len(resp.Identities) > 1 && isUsername:
    // The username case: we need to ignore the results that are prefixes of
    // the queried username. For example, if the filter is `jsmith`, we
    // ignore `jsmithson` and `jsmithers`.
    for _, identity := range resp.Identities {
        if identity.Name == filter {
            return &identity, nil
        }
    }
    return nil, fmt.Errorf("unexpected: browseIdentities(%s) returned two identities but none of them match the username exactly", filter)
case len(resp.Identities) > 1 && isAnEmail:
    // The email case: we do not need to filter out anything. So let's
    // arbitrarily return the first identity.
    return &resp.Identities[0], nil
}

[maelvls]

@luispresuelVenafi @marcos-albornoz Thank you for your reviews! I addressed your comments, please take another look 🙏

[maelvls]

⚠️ NOTE ⚠️: Please suspend reviewing this PR until I've had the chance to write some unit tests around getIdentity using the old implementation from the master branch.

Instead, look at https://github.com/Venafi/vcert/pull/420. Once #420 is merged into master, we can resume reviewing #418. You'll notice that none of the changes from #418 are included in #420, which should prevent any potential regressions.

Update: now resuming work on this PR since #420 has been merged.

[maelvls]

Marcos caught a regression that I introduced with my switch statement: when searching for foo, if the API simply returns foobar, the code would mistakenly return the identity foobar instead of returning an error.

I have added a test case to #420 and fixed the regression in 2340938.

[maelvls]

I rebased on top of master and also to GPG-sign the commits. One last review and we are done here! 🙏 @luispresuelVenafi

[maelvls]

The build passed.

https://jenkins.eng.venafi.com/job/VCert/job/vcert/3178/

[maelvls]

Good news, setpolicy now works with emails! Previously, it was only possible to use a username (e.g., mael.valais).

For example, let's imagine I want to configure the Contacts field of a given policy folder, I can do vcert setpolicy with an email:

{
  "users": [
    "mael.valais@venafi.com"
  ],
  "policy": {
    "wildcardAllowed": true,
    "certificateAuthority": "\\VED\\Policy\\Administration\\CAs\\Microsoft CA Web Server 1 Year",
    "keyPair": {
      "reuseAllowed": false
    },
    "subjectAltNames": {
      "dnsAllowed": true,
      "ipAllowed": true,
      "emailAllowed": true,
      "uriAllowed": true,
      "upnAllowed": true
    }
  },
  "defaults": {
    "keyPair": {
      "keyType": "RSA",
      "rsaKeySize": 2048,
      "serviceGenerated": false
    },
    "autoInstalled": false
  }
}

Unfortunately, vcert getpolicy will still return a username:

{
  "users": [
    "mvalais"
  ],
  "policy": {
    "wildcardAllowed": true,
    "certificateAuthority": "\\VED\\Policy\\Administration\\CAs\\Microsoft CA Web Server 1 Year",
    "keyPair": {
      "reuseAllowed": false
    },
    "subjectAltNames": {
      "dnsAllowed": true,
      "ipAllowed": true,
      "emailAllowed": true,
      "uriAllowed": true,
      "upnAllowed": true
    }
  },
  "defaults": {
    "keyPair": {
      "keyType": "RSA",
      "rsaKeySize": 2048,
      "serviceGenerated": false
    },
    "autoInstalled": false
  }
}

[maelvls]

I fixed the two problems we talked about in the meeting. This PR is now ready to be merged!

Jenkins build: https://jenkins.eng.venafi.com/job/VCert/job/vcert/3180/

Update: Not sure what happened, but the VaaS Unit Tests failed in 3180:

[2024-02-01T09:41:33.943Z] === RUN   TestSearchCertificate
[2024-02-01T09:41:34.202Z] vCert: 2024/02/01 09:41:34 Got 200 OK status for GET https://api.venafi.cloud/v1/useraccounts
[2024-02-01T09:41:34.460Z] vCert: 2024/02/01 09:41:34 Got 200 OK status for GET https://api.venafi.cloud/outagedetection/v1
/applications/Open%20Source%20Integrations/certificateissuingtemplates/Unrestricted
[2024-02-01T09:41:34.717Z] vCert: 2024/02/01 09:41:34 Got 200 OK status for GET https://api.venafi.cloud/outagedetection/v1
/applications/name/Open%20Source%20Integrations
[2024-02-01T09:41:34.974Z] vCert: 2024/02/01 09:41:34 Got 412 Precondition Failed status for POST https://api.venafi.cloud/
outagedetection/v1/certificaterequests
[2024-02-01T09:41:34.974Z]     connector_test.go:1127: vcert error: server error: Unexpected status code on Venafi Cloud zo
ne read. Status: 412 Precondition Failed
[2024-02-01T09:41:34.974Z]         Error Code: 10726 Error: Distinguished name component CN with value "t1706780494-caqr.ve
nafi.example.com" is invalid

I'll re-submit a build.

Update 2: https://jenkins.eng.venafi.com/job/VCert/job/vcert/3181/

[2024-02-01T19:39:34.415Z] --- FAIL: TestReadZoneConfiguration (0.62s)
[2024-02-01T19:39:34.415Z] --- FAIL: TestRequestCertificate (1.20s)
[2024-02-01T19:39:36.052Z] --- FAIL: TestRequestCertificateWithUsageMetadata (1.10s)
[2024-02-01T19:39:36.876Z] --- FAIL: TestRequestCertificateWithValidityHours (1.05s)
[2024-02-01T19:39:37.954Z] --- FAIL: TestRequestCertificateWithValidityDuration (1.11s)
[2024-02-01T19:39:39.035Z] --- FAIL: TestRetrieveCertificate (1.07s)
[2024-02-01T19:39:40.419Z] --- FAIL: TestRetrieveCertificateRootFirst (1.29s)
[2024-02-01T19:39:41.444Z] --- FAIL: TestGetCertificateStatus (1.00s)
[2024-02-01T19:39:41.968Z] --- FAIL: TestReadPolicyConfiguration (0.49s)
[2024-02-01T19:39:42.993Z] --- FAIL: TestRetireCertificate (1.12s)
[2024-02-01T19:39:44.375Z] --- FAIL: TestRetireCertificateWithPickUpID (1.22s)
[2024-02-01T19:39:45.397Z] --- FAIL: TestRetireCertificateTwice (1.24s)
[2024-02-01T19:39:52.803Z] --- FAIL: TestSearchCertificate (0.95s)
[2024-02-01T19:40:29.807Z] FAIL
[2024-02-01T19:40:29.807Z] FAIL github.com/Venafi/vcert/v5/pkg/venafi/cloud 63.668s

It seems like there is an issue with VaaS, but no issue related to my PR hopefully! 😅

[luispresuelVenafi]

Just validated the new commits. Looks Good To Me