redhat-developer / s2i-dotnetcore

.NET Core OpenShift images
Apache License 2.0
112 stars 192 forks source link

trust_ssl_dirs, realpath and the service CA #464

Closed cajosc closed 1 year ago

cajosc commented 1 year ago

We're running a .NET application which does HTTPS requests to other pods in the same Openshift cluster. These are secured using the service CA, and we add /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt to _DOTNET_SSLDIRS. The use of realpath here (and here) causes the symlink in /opt/app-root/ssl_dir to target service-ca.crt in the timestamped directory /var/run/secrets/kubernetes.io/serviceaccount/..YYYY-MM_DD_HH_mm_ss.NNNNNNNN instead.

Since /var/run/secrets/kubernetes.io/serviceaccount is a projected volume with, among other things, a serviceAccountToken source with an expiration time of 3607 seconds, the symlink stops working after that time when the timestamped directory is replaced.

spec:
  containers:
  - name: dotnet-app
    volumeMounts:
    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
      name: kube-api-access-chtld
      readOnly: true
...
  volumes:
  - name: kube-api-access-chtld
    projected:
      defaultMode: 420
      sources:
      - serviceAccountToken:
          expirationSeconds: 3607
          path: token
      - configMap:
          items:
          - key: ca.crt
            path: ca.crt
          name: kube-root-ca.crt
      - downwardAPI:
          items:
          - fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace
            path: namespace
      - configMap:
          items:
          - key: service-ca.crt
            path: service-ca.crt
          name: openshift-service-ca.crt

Is it really necessary to use realpath to resolve symlinks?

aslicerh commented 1 year ago

Ok, so there are a few different things here and let me make sure I understand what is going on and what is expected.

You have a file, /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt, that is added to DOTNET_SSL_DIRS. This file is a symlink to some other file /var/run/secrets/kubernetes.io/serviceaccount/..YYYY-MM_DD_HH_mm_ss.NNNNNNNN/service-ca.crt. This directory and file is temporary and is removed every so often and replaced with an updated file. And the symlink at /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt is updated when the temporary file is replaced? Is that correct? And so you want to have a symlink placed into DOTNET_SSL_CERT_DIR that links to the symlink at /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt instead of linking to the direct file?

I tested in a container that .NET would follow a link of a link like that and it seemed fine. I should probably run a test that it works with the link target changing while the app is running.

From the container side of things, I think we can probably just use no-symlinks with realpath, which should cause us to make links to the symlinks, instead of getting their target paths. I'm working on testing this out now, though it is a little tricky to set up a small, local test of it since podman copied symlinks as files into the containers when building them.

tmds commented 1 year ago

I think we can probably just use no-symlinks with realpath, which should cause us to make links to the symlinks, instead of getting their target paths.

Yes, this is the proper fix. We don't want to expand the symlinks. realpath is used here just to turn relative paths into absolute ones.

tmds commented 1 year ago

DOTNET_SSL_DIRS was meant to be aware of updates to the provided paths. This is why these get created as symlinks in the SSL_CERT_DIR directory that is provided to .NET. It was an oversight the paths provided may include symlinks already and realpath would eliminate those.

@cajosc thanks for the clear bug report!

aslicerh commented 1 year ago

The change for this has been merged. It should start showing up in containers at the next .NET release.

shupoval commented 11 months ago

@aslicerh, thanks for implementing this!

Does it mean that even though this change was for .net 6/7 it will only be released in official images after the .net 8 release? Or may be I misinterpreted your previous statement?

I'm curious if there is a way to correlate which changes (e.g. like the one from this issue) are included into a particular version of the image.

I see that the recent update to the .net 6.0 images happened a few days ago, but it is unclear what actually changed there. I noticed that the image has "vcf-ref" label, but, unfortunately, the hash value from that label doesn't point to any commit in this repo.

I appreciate any help/comments with regard to issues raised in this comment :)

Thanks in advance!

tmds commented 11 months ago

You can run:

podman run --pull always registry.access.redhat.com/ubi8/dotnet-60 cat /opt/app-root/etc/trust_ssl_dirs

When you do, you'll see the --no-symlinks option that was added in https://github.com/redhat-developer/s2i-dotnetcore/pull/466 is not yet present.

The intent was to include this in the monthly updates for .NET 6 and .NET 7 but that has not yet happened.

@aslicerh when will this be available?

aslicerh commented 11 months ago

This should of been merged into the official RH branches but it hasn't been, and that's my fault. It'll be in the next release, which should be next week, or maybe the week after depending on infra and people outages.

shupoval commented 11 months ago

@tmds, @aslicerh, thanks a lot for such a quick reply.

Just out of curiosity, these "... official RH branches ..." are part of some other non-public repo?

I'm asking because I found that images are being labeled with "vcf-ref" which most likely contains sha of a commit based on which that build was made. The only problem I found, the value in that label wasn't resolved to any valid commit in this repo.

aslicerh commented 11 months ago

@shupoval There's an internal build repo that RH uses. It is based on this repo, but has additions for RH build and test pipeline configuration. Nothing that changes the functionality, just things related to internal RH infrastructure. Unfortunately, that does mean that the sha's between the two are not the same.

Theoretically, any changes pushed here (that are non-breaking) should make it into the official images in the next release, maybe the one after just depending on timing, compose freezes, build-freezes, etc. But it is a manual process...

aslicerh commented 11 months ago

@shupoval This change is now in the latest container release that just went out a few minutes ago.