Set the ldap certificate value properly.

sandyg1 commented 5 years ago

Set the ldap certificate value properly. Also added a few comments to clarify the group attributes.

sandyg1 commented 5 years ago

Added DCO

brightzheng100 commented 5 years ago

This is an interesting thing while working with certificate.

For demonstration purposes, I've tested out with 3 scenarios:

Option 1

In ldap.yml -- asuming we keep current ops file as is:

ca_cert: ((ldap_ca_cert))

Deployment:

$ credhub set -n /bosh-lite/concourse4/ldap_ca_cert -t certificate --certificate ~/workspaces/CA-CERTS/certs/ca.crt.pem
$ bosh deploy -d concourse4 concourse.yml \
  ...
  -v ldap_ca_cert="((ldap_ca_cert))" \
  ...

It works.

Option 2

In ldap.yml, if we accept the @vito's suggestion:

ca_cert:
  certificate: ((ldap_ca_cert))

Deployment:

$ bosh deploy -d concourse4 concourse.yml \
  ...
  --var-file ldap_ca_cert=~/workspaces/CA-CERTS/certs/ca.crt.pem \
  ...

It works too.

Option 3

ldap.yml:

ca_cert:
  certificate: ((ldap_ca_cert))

Deployment:

$ credhub set -n /bosh-lite/concourse4/ldap_ca_cert -t certificate --certificate ~/workspaces/CA-CERTS/certs/ca.crt.pem
$ bosh deploy -d concourse4 concourse.yml \
  ...
  -v ldap_ca_cert="((ldap_ca_cert.certificate))" \
  ...

It also works.

Frankly, I'd say the way @vito highlighted is more precise.

Meanwhile, I'll reach out further to some other teams to get back the "best practice".

More feedback?

vito commented 5 years ago

@brightzheng100 Thanks for the investigation!

Come to think of it, I think -v and --var-file also let you set individual fields, so even if you have ca_cert: ((ldap_ca_cert)), I think you can do this:

$ bosh deploy -d concourse4 concourse.yml \
  ...
  --var-file ldap_ca_cert.certificate=~/workspaces/CA-CERTS/certs/ca.crt.pem \
  ...

At least, I remember that working with -v. I haven't tried with --var-file.

@sandyg1 Does the above work for you?

mjenk664 commented 5 years ago

Hi @sandyg1 @vito,

I am working with a customer right now who is looking to set up Concourse w/ LDAP Auth. We ran into this same challenges as Vito above. Having only the ca_cert: ((ldap_ca_cert)) property defined fails to actually add in the certificate.

I came across this PR and we tested and validated that this indeed is the culprit. After adding the certificate: field to the ca_cert: property, things are working properly :)

It's been a while since this PR has received any activity, so I thought I'd help others out. It'd be great if this could get merged in for those who want to configure ldap in the future.

vito commented 5 years ago

@marijenk Thanks for the update. I feel like this is a documentation issue, though, and we shouldn't merge this PR. We've actually received a couple other similar PRs in the past which have been closed with the same reasoning:

These ops files expect typed variables to be specified for these properties, as the BOSH property specifies it as type: certificate.

This allows the value to be specified using a generated var or provided by a certificate-typed credential in CredHub. If we hardcode certificate: ((foo)) they can only be specified as string values, which seems like a bit of a shame. Technically these fields are typically satisfied by a static value, making the generated vars use case a bit moot, but it still seems like a good practice to use typed values when possible if you're e.g. storing credentials in CredHub.

This has come up three times now in the past couple of weeks, though, so I think something should change. It feels like we're using the BOSH abstractions correctly, so I guess we just need to update documentation...somewhere. I'm not sure where. 😕 Is there anywhere you would have looked to see that kind of thing?

Closing; I've opened an issue for updating documentation here: https://github.com/concourse/concourse-bosh-deployment/issues/163

Thanks!

concourse / concourse-bosh-deployment