InseeFrLab / images-datascience

Collection of Docker images to build the data science catalog of the Onyxia project
MIT License
24 stars 23 forks source link

onyxia-init.sh / Vault : add optional mode for service account based auth (+ other fixes) #197

Closed phlg closed 7 months ago

phlg commented 7 months ago

Hello,

This PR is pushing forward a new feature as well as fixing a couple of implementation flaws related to Vault management in onyxia-init.sh. Both are detailed below in distinct sections.

Feature

Feature-wise, we aim to allow for a secondary mode of authentication using the JWT generated for the service account used for the pod a service is running in. The relevant part in the changes is this :

    if [[ "$VAULT_INJECTION_SA_ENABLED" = "true" && "$VAULT_INJECTION_SA_MODE" = "jwt" ]]; then
        echo "using service account injection jwt to get vault token"
        VAULT_TOKEN=$(vault write -field="token" auth/$VAULT_INJECTION_SA_AUTH_PATH/login role=$VAULT_INJECTION_SA_AUTH_ROLE jwt=@/var/run/secrets/kubernetes.io/serviceaccount/token)
    fi

This supposes the presence of four "new" environment variables, whose names were set arbitrarily, and can be changed if needed :

These variables mirror the not-yet-existing region parameters suggested in this issue : https://github.com/InseeFrLab/onyxia-api/issues/409. How these variables are to be set in the interactive-services is out of this scope, but the probable candidate would be to change the Helm charts to support them (values.schema.json to inject default from region into values + add a new CM or Secret to actually set the environment variables).

Also related to Vault is a minor change in the implementation : adding a new if [[ -n "$VAULT_TOKEN" ]] test prior to querying Vault - this should be transparent to existing deployments of Onyxia, but prevents errors that could happen if the SA auth method is misconfigured for instance.

The SA based auth promoted in this PR was tested with the vault-prefix tag of onyxia-api advertised in this issue : https://github.com/InseeFrLab/onyxia/issues/783

Fixes

Unrelated to Vault are a couple of fixes in the code :

https://github.com/InseeFrLab/images-datascience/blob/1dd43022373a2a3adf74ab79cd5a5780ef54ee0b/scripts/onyxia-init.sh#L43-L55

This has the drawback of "eating" the quotes, resulting on problems for secrets whose value contain a whitespace :

image

(though this problem appears to be limited to the "sudo-less" mode ; secrets defined in /etc/environment appear to be parsed correctly even without quotes).

To counter this, I replaced echo with a printf command, while defining a $value variable to remove some copy/pasting.

https://github.com/InseeFrLab/images-datascience/blob/1dd43022373a2a3adf74ab79cd5a5780ef54ee0b/scripts/onyxia-init.sh#L46

In its current implementation, on any service not running R, $R_HOME is evaluated to an empty string, which in turn causes the test to check if /etc exists, which it always does. In other words, every service - with or without R - attempted to append to Renviron.site. For a R based service, it works as intended, but for the others, it in fact creates (or attempts to create) a /etc/Renviron.site file.

I replaced that test with a different one, used in a different location in onyxia-init.sh : if command -v R;