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

⚠️ Store htpasswd files in Secrets instead of ConfigMaps #1241

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

lentzi90
Copy link
Member

@lentzi90 lentzi90 commented Mar 29, 2023

What this PR does / why we need it:

The htpasswd files for Ironic and Inspector contains clear text usernames and hashed passwords so it is better to store them in Secrets.

Depending on how exactly Ironic is deployed this could be a breaking change that requires manual action from the user.
I have tested this with the deploy.sh script and confirmed that it is working. Re-deploying Ironic, with the updated kustomization using the script, automatically creates the new Secrets and configures Ironic and Inspector to use them instead of the ConfigMaps.

Note that the ConfigMaps are not automatically removed. Ideally, the user should remove the ConfigMaps and change the credentials. The ConfigMaps in question are named baremetal-operator-ironic-htpasswd-<random-hash> and baremetal-operator-ironic-inspector-htpasswd-<random-hash> and are located in the baremetal-operator-system Namespace by default.

Note that if the credentials are changed, they must also be updated for BMO. This can be done in the same way by re-deploying using the script.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Steps for changing ConfigMap to Secret

Before you start, confirm if the environment is affected or not.
This can be done with the following commands:

kubectl -n baremetal-operator-system get deployments.apps baremetal-operator-ironic \
  -o jsonpath="{.spec.template.spec.containers[?(@.name == 'ironic-httpd')].envFrom}{'\n'}"
kubectl -n baremetal-operator-system get deployments.apps baremetal-operator-ironic \
  -o jsonpath="{.spec.template.spec.containers[?(@.name == 'ironic')].envFrom}{'\n'}"
kubectl -n baremetal-operator-system get deployments.apps baremetal-operator-ironic \
  -o jsonpath="{.spec.template.spec.containers[?(@.name == 'ironic-inspector')].envFrom}{'\n'}"

Affected environments would have configMapRefs for the htpasswd files in the output.
If it instead shows secretRefs for the htpasswd files it is not affected.

Start by listing the current ConfigMaps and Secrets for reference, so it is easy to check which are new and which are old later.

kubectl -n baremetal-operator-system get secrets > old-secrets.txt
kubectl -n baremetal-operator-system get configmaps > old-configmaps.txt

Generate new credentials and configuration files, for example like this:

IRONIC_USERNAME="$(tr -dc 'a-zA-Z0-9' < /dev/urandom | fold -w 12 | head -n 1)"
IRONIC_PASSWORD="$(tr -dc 'a-zA-Z0-9' < /dev/urandom | fold -w 12 | head -n 1)"
IRONIC_INSPECTOR_USERNAME="$(tr -dc 'a-zA-Z0-9' < /dev/urandom | fold -w 12 | head -n 1)"
IRONIC_INSPECTOR_PASSWORD="$(tr -dc 'a-zA-Z0-9' < /dev/urandom | fold -w 12 | head -n 1)"

echo "IRONIC_HTPASSWD=$(htpasswd -n -b -B "${IRONIC_USERNAME}" "${IRONIC_PASSWORD}")" > ironic-htpasswd
echo "INSPECTOR_HTPASSWD=$(htpasswd -n -b -B "${IRONIC_INSPECTOR_USERNAME}" \
        "${IRONIC_INSPECTOR_PASSWORD}")" > ironic-inspector-htpasswd

cat > ironic-auth-config <<-EOF
[ironic]
auth_type=http_basic
username=${IRONIC_USERNAME}
password=${IRONIC_PASSWORD}
EOF

cat > ironic-inspector-auth-config <<-EOF
[inspector]
auth_type=http_basic
username=${IRONIC_INSPECTOR_USERNAME}
password=${IRONIC_INSPECTOR_PASSWORD}
EOF

Create new secrets from htpasswd files, authentication configuration files and credentials for BareMetalOperator.
The exact names of these do not matter and the ones provided here are just examples.

# Htpasswd
kubectl -n baremetal-operator-system create secret generic baremetal-operator-ironic-htpasswd \
  --from-env-file=ironic-htpasswd
kubectl -n baremetal-operator-system create secret generic baremetal-operator-ironic-inspector-htpasswd \
  --from-env-file=ironic-inspector-htpasswd
# Auth config
kubectl -n baremetal-operator-system create secret generic baremetal-operator-ironic-auth-config \
  --from-file=auth-config=ironic-auth-config
kubectl -n baremetal-operator-system create secret generic baremetal-operator-ironic-inspector-auth-config \
  --from-file=auth-config=ironic-inspector-auth-config
# BMO credentials
kubectl -n baremetal-operator-system create secret generic ironic-credentials \
  --from-literal=username="${IRONIC_USERNAME}" --from-literal=password="${IRONIC_PASSWORD}"
kubectl -n baremetal-operator-system create secret generic ironic-inspector-credentials \
  --from-literal=username="${IRONIC_INSPECTOR_USERNAME}" --from-literal=password="${IRONIC_INSPECTOR_PASSWORD}"

Edit the Ironic Deployment to make it use the Secrets instead of the ConfigMaps.
Unfortunately, it is not easy to generate a fool proof patch (for kubectl patch) for this, since the envFrom field is an array.
It is only possible to replace the whole array or do index based replacements, which cannot really be used without knowing the exact details of the specific environment.
For this reason we only show the steps using kubectl edit instead of providing a patch that would likely not work for everyone.

Edit the Deployment with this command: kubectl -n baremetal-operator-system edit deployment baremetal-operator-ironic.
Note that the names of the ConfigMaps may differ depending on version and how it was deployed (e.g. the baremetal-operator- prefix may not be included in v0.1.2).
The order of the containers and other fields can also differ.
The necessary changes are described here:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: baremetal-operator-ironic
spec:
  template:
    spec:
      containers:
      # Only the containers ironic-httpd, ironic and ironic-inspector need changes
      - name: ironic-httpd
        envFrom:
        # Add the secretRefs like this
        - secretRef:
            name: baremetal-operator-ironic-htpasswd
        - secretRef:
            name: baremetal-operator-ironic-inspector-htpasswd
        # Remove these old configMapRefs
        - configMapRef:
            name: baremetal-operator-ironic-htpasswd-<random-hash>
        - configMapRef:
            name: baremetal-operator-ironic-inspector-htpasswd-<random-hash>
      - name: ironic
        envFrom:
        # Add the secretRef like this
        - secretRef:
            name: baremetal-operator-ironic-htpasswd
        # Remove the old configMapRef
        - configMapRef:
            name: baremetal-operator-ironic-htpasswd-<random-hash>
      - name: ironic-inspector
        envFrom:
        # Add the secretRef like this
        - secretRef:
            name: baremetal-operator-ironic-inspector-htpasswd
        # Remove the old configMapRef
        - configMapRef:
            name: baremetal-operator-ironic-inspector-htpasswd-<random-hash>
      volumes:
      # Change the secret name for the ironic-auth-config and ironic-inspector-auth-config
      # to match what was created earlier.
      - name: ironic-auth-config
        secret:
          defaultMode: 420
          secretName: baremetal-operator-ironic-auth-config
      - name: ironic-inspector-auth-config
        secret:
          defaultMode: 420
          secretName: baremetal-operator-ironic-inspector-auth-config

Now patch the BareMetalOperator Deployment to make use of the new credentials.

cat > patch.yaml <<-EOF
apiVersion: apps/v1
kind: Deployment
metadata:
  name: baremetal-operator-controller-manager
spec:
  template:
    spec:
      volumes:
      - name: ironic-credentials
        secret:
          defaultMode: 420
          secretName: ironic-credentials
      - name: ironic-inspector-credentials
        secret:
          defaultMode: 420
          secretName: ironic-inspector-credentials
EOF
kubectl -n baremetal-operator-system patch deployment baremetal-operator-controller-manager --patch-file=patch.yaml

Check that the new Pods become Running and then delete the old ConfigMaps and Secrets (optional).

# Check that the Pods become Running
kubectl -n baremetal-operator-system get pods
# Delete old ConfigMaps (check old-configmaps.txt if you are unsure)
kubectl -n baremetal-operator-system delete configmap baremetal-operator-ironic-htpasswd-<random-hash>
kubectl -n baremetal-operator-system delete configmap baremetal-operator-ironic-inspector-htpasswd-<random-hash>
# Delete old Secrets (check old-secrets.txt if you are unsure)
kubectl -n baremetal-operator-system delete secret baremetal-operator-ironic-auth-config-<random-hash>
kubectl -n baremetal-operator-system delete secret baremetal-operator-ironic-inspector-auth-config-<random-hash>
kubectl -n baremetal-operator-system delete secret ironic-credentials-<random-hash>
kubectl -n baremetal-operator-system delete secret ironic-inspector-credentials-<random-hash>

Confirm the fix by again checking the output of these commands:

kubectl -n baremetal-operator-system get deployments.apps baremetal-operator-ironic \
  -o jsonpath="{.spec.template.spec.containers[?(@.name == 'ironic-httpd')].envFrom}{'\n'}"
kubectl -n baremetal-operator-system get deployments.apps baremetal-operator-ironic \
  -o jsonpath="{.spec.template.spec.containers[?(@.name == 'ironic')].envFrom}{'\n'}"
kubectl -n baremetal-operator-system get deployments.apps baremetal-operator-ironic \
  -o jsonpath="{.spec.template.spec.containers[?(@.name == 'ironic-inspector')].envFrom}{'\n'}"

They should now show only secretRefs for htpasswd files.

Clean up temporary files:

rm patch.yaml
rm old-secrets.txt
rm old-configmaps.txt
shred -u ironic-htpasswd
shred -u ironic-inspector-htpasswd
shred -u ironic-auth-config
shred -u ironic-inspector-auth-config

The htpasswd files for Ironic and Inspector contains clear text
usernames and hashed passwords so it is better to store them in Secrets.

Depending on how exactly Ironic is deployed this could be a breaking
change that requires manual action from the user.
I have tested this with the
[deploy.sh](https://github.com/metal3-io/baremetal-operator/blob/main/tools/deploy.sh)
script and confirmed that it is working. Re-deploying Ironic, with the
updated kustomization using the script, automatically creates the new
Secrets and configures Ironic and Inspector to use them instead of the
ConfigMaps.

Note that the ConfigMaps are **not** automatically removed. Ideally, the
user should remove the ConfigMaps and change the credentials. The
ConfigMaps in question are named
`baremetal-operator-ironic-htpasswd-<random-hash>` and
`baremetal-operator-ironic-inspector-htpasswd-<random-hash>` and are
located in the `baremetal-operator-system` Namespace by default.

Note that if the credentials are changed, they must also be updated for
BMO. This can be done in the same way by re-deploying using the script.
@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 29, 2023
@lentzi90
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@tuminoid tuminoid self-requested a review March 29, 2023 08:19
@dtantsur
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2023
@lentzi90
Copy link
Member Author

/hold
Until I have time to test and document manual steps for patching a live environment.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2023
@lentzi90
Copy link
Member Author

/hold cancel

I have added manual steps for live environments in the description. It is hard to make something fool proof without making too many assumptions, so it is unfortunately quite general. The users will need to adapt to their specific environments.

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 30, 2023
@tuminoid
Copy link
Member

/hold cancel

I have added manual steps for live environments in the description. It is hard to make something fool proof without making too many assumptions, so it is unfortunately quite general. The users will need to adapt to their specific environments.

Thanks @lentzi90, the steps are detailed enough and quite OK. Anyone installing BMO should be able to follow those.

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@lentzi90
Copy link
Member Author

Prow/Tide test, please disregard
/test unit

@lentzi90
Copy link
Member Author

/test unit

@lentzi90
Copy link
Member Author

@kashifest @zaneb @dtantsur please take a look when you have the time.

@kashifest
Copy link
Member

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest, tuminoid

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2023
@kashifest
Copy link
Member

/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2023
@kashifest
Copy link
Member

Rerunning the tests since the previous runs were 2 weeks old. Unhold it when the tests pass.
/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@kashifest
Copy link
Member

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2023
@metal3-io-bot metal3-io-bot merged commit ae34635 into metal3-io:main Apr 14, 2023
@lentzi90 lentzi90 deleted the lentzi90/secret-htpasswd branch April 14, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants