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

fix: Use Opaque secret for TLS config #191

Merged
merged 3 commits into from
Jul 4, 2022

Conversation

Callisto13
Copy link
Member

@Callisto13 Callisto13 commented Jun 29, 2022

This is a collection of final fixes for TLS:

feat: Document that TLS secret is Opaque type

We realised that using a tls secret type and adding on a ca.crt key
would be annoying for users. With the tls.crt and tls.key fields,
users have the options to supply the values without the START and END
headers/footers, which are added back by the Secrets controller. For the
ca.crt, we would require that the headers are present (adding the pem
to the CaPool with fail otherwise) and we don't want to have to
process the bytes ourselves.
Users can create the other fields with or without headers, but one rule for
part of the data, and another rule for the rest is an irritating thing to
document so this just makes it easier.
Certmanager creates all data values with the headers present, so this
will work for either users in either case.


ref: Save TLS config data as bytes

Loading the TLS certs and CaPool takes []byte anyway, so there is no
need to string() and []byte() back and forth.

And load 509X from bytes not files.

Also stop decoding the secret data: I did not realised that was done
magically when you Get the secret.

Finally add a bit more logging to make TLS load failures clearer.


feat: RequireTransportSecurity when TLS is not nil

We only want to require this when TLS has actually been configured,
otherwise the client call will fail.


Tested with Flintlock:

# in flintlock generate certs
./hack/scripts/gen_local_certs.sh all \
  --host 192.168.0.31 \
  --cfssl $(which cfssl) \
  --cfssljson $(which cfssljson) \
  --output tls-test

# start server
sudo ./bin/flintlockd run \
  --containerd-socket=/run/containerd-dev/containerd.sock \
  --parent-iface="${NET_DEVICE}" \
  --grpc-endpoint=0.0.0.0:9091 \
  --tls-cert tls-test/192.168.0.31.pem \
  --tls-key tls-test/192.168.0.31-key.pem \
  --tls-client-ca tls-test/intermediate-ca.pem \
  --tls-client-validate \
  --basic-auth-token foo

# in capi create cluster and start tilt
export CAPI_KIND_CLUSTER_NAME=capmvm-test
kind create cluster --name $CAPI_KIND_CLUSTER_NAME
tilt --host 0.0.0.0 up

# create secret with TLS config
kubectl create secret generic mytlssecret \
  --from-file=tls.crt=tls-test/liquidmetalclient1.pem \
  --from-file=tls.key=tls-test/liquidmetalclient1-key.pem \
  --from-file=ca.crt=tls-test/intermediate-ca.pem

# create secret with basic auth token
kubectl create secret generic foo --from-literal 192.168.0.31=foo

# generate a template with fields
cat cluster.yaml
...
spec:
  tlsSecretRef: mytlssecret
  placement:
    staticPool:
      basicAuthSecret: mybasicsecret
      hosts:
      - controlplaneAllowed: true
        endpoint: 192.168.0.31:9091
...

# and create
kubectl apply -f cluster.yaml

We realised that using a `tls` secret type and adding on a `ca.crt` key
would be annoying for users. With the `tls.crt` and `tls.key` fields,
users have the options to supply the values without the `START` and `END`
headers/footers, which are added back by the Secrets controller. For the
`ca.crt`, we would require that the headers are present (adding the pem
to the `CaPool` with fail otherwise) and we don't want to have to
process the bytes ourselves.
Users can create the other fields with or without headers, but one rule for
part of the data, and another rule for the rest is an irritating thing to
document so this just makes it easier.
Certmanager creates all data values with the headers present, so this
will work for either users in either case.
Loading the TLS certs and CaPool takes `[]byte` anyway, so there is no
need to `string()` and `[]byte()` back and forth.

Also stop decoding the secret data: I did not realised that was done
magically when you `Get` the secret.

Finally add a bit more logging to make TLS load failures clearer.
We only want to require this when TLS has actually been configured,
otherwise the client call will fail.
@Callisto13 Callisto13 added the kind/bug Something isn't working label Jun 29, 2022
@Callisto13 Callisto13 merged commit 9a4070a into liquidmetal-dev:main Jul 4, 2022
@Callisto13 Callisto13 deleted the final-auth-fixes branch July 4, 2022 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants