Switch ironic configuration rendering to Jinja2 #201

maelk · 2020-09-20T06:51:30Z

This centralizes the cryptic configuration done in multiple files. It also removes all calls to crudini, that was remove options set in a section if it was set to the same value in [DEFAULT].

The type of deployment of Ironic is set at the beginning of each script in IRONIC_DEPLOYMENT :

runironic.sh -> Combined
runironic-api.sh -> API
runironic-conductor.sh -> Conductor

The configuration file is templated based on this variable.

In addition, it configures json_rpc to be authenticated when API and conductor are running in the same container and
HTTP_BASIC_HTPASSWD is set. API and json_rpc would accept the same credentials. This is to prevent other processes in the same host to access the RPC interface over localhost. The LISTEN_ALL_INTERFACES environment variable is added to allow restricting on which IP address ironic listens to for API and RPC.

maelk · 2020-09-20T06:51:59Z

This should make #200 unnecessary.

maelk · 2020-09-20T06:52:07Z

/test-integration

maelk · 2020-09-21T04:11:15Z

/test-integration

maelk · 2020-09-21T04:57:50Z

/test-integration

maelk · 2020-09-21T05:05:31Z

/test-integration

maelk · 2020-09-21T13:45:21Z

/assign @dtantsur
/cc @zaneb
/cc @dhellmann

dhellmann

This looks OK, but I'm starting to wonder if we would be better off building an operator to manage Ironic instead of putting all of this logic in the container entry point.

One question inline about how the auth strategy is detected.

dhellmann · 2020-09-21T13:45:51Z

ironic.conf.j2

@@ -1,5 +1,10 @@
 [DEFAULT]
+{% if "HTTP_BASIC_HTPASSWD" in env %}


Should we check that the value isn't empty?

good point. I'll update.

This centralizes the cryptic configuration done in multiple files. In addition, it configures json_rpc to be authenticated when API and conductor are running in the same container and HTTP_BASIC_HTPASSWD is set. API and json_rpc would accept the same credentials. This is to prevent other processes in the same host to access the RPC interface over localhost.

maelk · 2020-09-21T14:48:39Z

/test-integration

dhellmann · 2020-09-21T14:53:06Z

/approve

zaneb

This looks good to me.

zaneb · 2020-09-21T20:51:16Z

ironic.conf.j2

+# containers are in host networking.
+{% if ( env.IRONIC_DEPLOYMENT == "Combined" or env.IRONIC_DEPLOYMENT == "Conductor" ) and ( "HTTP_BASIC_HTPASSWD" in env and env.HTTP_BASIC_HTPASSWD | length ) %}
+auth_strategy = http_basic
+http_basic_auth_user_file = /etc/ironic/htpasswd


This is better than nothing for now, but if the combined container is going to continue to be a thing then I think we want to use separate credentials for json-rpc (could easily be generated inside the container, to avoid complicating the interface).

I have some on-going work in BMO to stop using the combined version. I'd like to keep support for it in this image for now to make sure that it is backwards compatible, but when the BMO change will have been there for a while, we can move to deprecate it.

I will add json-rpc credentials generation

maelk · 2020-09-22T06:59:24Z

I have now added the credentials generation for the RPC. I have considered the following use cases. I have introduced a new variable HTTP_BASIC_HTPASSWD_RPC. The user can provide HTTP_BASIC_HTPASSWD and HTTP_BASIC_HTPASSWD_RPC. If

we are running conductor and HTTP_BASIC_HTPASSWD_RPC is not set but HTTP_BASIC_HTPASSWD is,
fallback to use HTTP_BASIC_HTPASSWD for RPC, to preserve compatibility.
we are running combined API and conductor, and HTTP_BASIC_HTPASSWD_RPC is set, use it
we are running combined API and conductor, and HTTP_BASIC_HTPASSWD_RPC is unset, but HTTP_BASIC_HTPASSWD
is, i.e. API is authenticated. We want to authenticate RPC by default, but the user might override with auth-config files.
Then try to infere the authentication strategy and credentials from /auth/ironic-rpc/auth-config.
If not present, then generate a username and password, create the config file and set
HTTP_BASIC_HTPASSWD_RPC

It is implemented in the third commit of this PR.
@zaneb How does this look to you ?

maelk · 2020-09-22T06:59:32Z

/test-integration

maelk · 2020-09-24T12:47:02Z

/test-integration
@dtantsur @dhellmann @derekhiggins @hardys How does this look to you ?

dhellmann · 2020-09-24T19:10:32Z

This looks clean to me. I think the other reviewers are more familiar with the way the actual settings are used, so I'll leave it open for one of them to look at.

/approve

zaneb · 2020-09-25T02:53:18Z

configure-ironic.sh

+                fi
+            # We do not have an auth config file, so we generate one
+            else
+                IRONIC_RPC_TMP_USERNAME="$(tr -dc 'a-zA-Z0-9' < /dev/urandom | fold -w 12 | head -n 1)"


There's no need for the username to be random, I'd suggest just calling it rpc-user or something.

ok, I'll change it!

zaneb · 2020-09-25T02:56:56Z

configure-ironic.sh

 if [ -n "${HTTP_BASIC_HTPASSWD}" ]; then
    printf "%s\n" "${HTTP_BASIC_HTPASSWD}" >"${HTPASSWD_FILE}"
+
+    if [ -z "${HTTP_BASIC_HTPASSWD_RPC:-}" ]; then


We think the combined mode is going away eventually, so I would be inclined not to make this part of the container interface. It's also redundant with the auth-config file.

Instead we could just always generate a password in combined deployments unless the auth-config file exists, and always use $HTTP_BASIC_HTPASSWD as HTTP_BASIC_HTPASSWD_RPC in conductor deployments.

You already did the hard work of implementing generation so we might as well use that to keep the interface that we have to maintain simple :)

ok, let me give it try.

I have now removed that variable. I kept the behaviour that in case of "Combined" we try to get the authentication info from the auth-config and if it is not provided, we generate some

zaneb · 2020-09-25T03:04:17Z

configure-ironic.sh

+                IRONIC_RPC_TMP_PASSWORD="$(tr -dc 'a-zA-Z0-9' < /dev/urandom | fold -w 12 | head -n 1)"
+                mkdir -p "/auth/ironic-rpc"
+                cat << EOF > "/auth/ironic-rpc/auth-config"
+[json-rpc]


Maybe both work(?), but it's json_rpc in the code. I think we should standardise on that, because in other places (like the crudini commands on line 53-54) it probably does matter whether it's - or _.

that's definitely a typo! thanks for the catch.

zaneb · 2020-09-25T03:15:55Z

configure-ironic.sh

+                IRONIC_RPC_TMP_USERNAME="$(tr -dc 'a-zA-Z0-9' < /dev/urandom | fold -w 12 | head -n 1)"
+                IRONIC_RPC_TMP_PASSWORD="$(tr -dc 'a-zA-Z0-9' < /dev/urandom | fold -w 12 | head -n 1)"
+                mkdir -p "/auth/ironic-rpc"
+                cat << EOF > "/auth/ironic-rpc/auth-config"


One thing I didn't think about before is that we're writing this to a file that is not on a tmpfs mount (like secrets are), so it could get written to disk at some point. Probably not a reason not to do this, but bindmounting it in from a secret is definitely more secure.

I agree that it is not great, but if we want to differentiate the RPC username/password from the API ones, then in the combined case, there is no other option that writing to disk if the user does not provide the credentials. If the user provides the auth-config, then that's more secure.

maelk · 2020-09-25T06:53:16Z

/test-integration

zaneb · 2020-09-25T16:13:56Z

configure-ironic.sh

+
+        # Populate HTTP_BASIC_HTPASSWD_RPC
+        if [ -n "${IRONIC_RPC_TMP_USERNAME:-}" ]; then
+            printf "%s\n" "$(htpasswd -n -b -B "${IRONIC_RPC_TMP_USERNAME}" "${IRONIC_RPC_TMP_PASSWORD}")" >"${HTPASSWD_FILE}-rpc"


I don't think the printf is necessary here, you can just do

htpasswd -n -b -B "${IRONIC_RPC_TMP_USERNAME}" "${IRONIC_RPC_TMP_PASSWORD}" >"${HTPASSWD_FILE}-rpc"

zaneb · 2020-09-25T16:40:43Z

configure-ironic.sh

+        printf "%s\n" "${HTTP_BASIC_HTPASSWD}" >"${HTPASSWD_FILE}-rpc"
+    fi
+else
+    export JSON_RPC_AUTH_STRATEGY="noauth"


I think we should move the code for the combined deployment type outside of this if/else, since we don't want to set noauth for Combined deployments just because basic_auth wasn't enabled on the API. So something like:

export JSON_RPC_AUTH_STRATEGY="noauth" if [ -n "${HTTP_BASIC_HTPASSWD}" ]; then if [ "${IRONIC_DEPLOYMENT}" == "Conductor" ]; then JSON_RPC_AUTH_STRATEGY="http_basic" printf "%s\n" "${HTTP_BASIC_HTPASSWD}" >"${HTPASSWD_FILE}-rpc" else printf "%s\n" "${HTTP_BASIC_HTPASSWD}" >"${HTPASSWD_FILE}" fi fi if [ "${IRONIC_DEPLOYMENT}" == "Combined" ]; then JSON_RPC_AUTH_STRATEGY="http_basic" # Current lines 45-74 fi

When running both API and Conductor in the same container, we'll generate the username and password if not given by the user through the ironic-rpc auth config file.

maelk · 2020-09-26T07:11:49Z

@zaneb Thank you for the comments. I updated the PR.
/test-integration

derekhiggins · 2020-09-28T15:07:48Z

/approve

works with installer, tested without and also with a rpc credentials passed in

metal3-io-bot · 2020-09-28T15:07:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekhiggins, dhellmann, maelk

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:

~~OWNERS~~ [derekhiggins,dhellmann]

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

maelk · 2020-09-28T15:26:25Z

Great! Is this ready to go in then ? If so please lgtm.

zaneb · 2020-09-28T21:20:37Z

LGTM but I am not a reviewer for this repo.

maelk · 2020-09-29T05:30:36Z

@dhellmann or @dtantsur Would you mind lgtm if lgty ? thank you!

derekhiggins · 2020-09-29T07:22:40Z

/lgtm

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 20, 2020

This was referenced Sep 20, 2020

Explicitly set json_rpc.auth_strategy to noauth #196

Closed

Add TLS support in Ironic configuration #198

Merged

maelk force-pushed the jinja2-mael branch from a448b33 to f8d1c99 Compare September 21, 2020 04:57

metal3-io-bot assigned dtantsur Sep 21, 2020

metal3-io-bot requested review from dhellmann and zaneb September 21, 2020 13:45

dhellmann reviewed Sep 21, 2020

View reviewed changes

maelk force-pushed the jinja2-mael branch from f8d1c99 to 7a862c3 Compare September 21, 2020 14:47

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2020

zaneb reviewed Sep 21, 2020

View reviewed changes

maelk force-pushed the jinja2-mael branch from acdd381 to 32400c8 Compare September 24, 2020 12:43

zaneb reviewed Sep 25, 2020

View reviewed changes

maelk force-pushed the jinja2-mael branch from 32400c8 to 2bb2ac0 Compare September 25, 2020 06:50

zaneb reviewed Sep 25, 2020

View reviewed changes

Add username and password generation for RPC

a678e4d

When running both API and Conductor in the same container, we'll generate the username and password if not given by the user through the ironic-rpc auth config file.

maelk force-pushed the jinja2-mael branch from 2bb2ac0 to a678e4d Compare September 26, 2020 07:09

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2020

metal3-io-bot merged commit 22df337 into metal3-io:master Sep 29, 2020

derekhiggins mentioned this pull request Sep 30, 2020

Setting UPSTREAM_IRONIC=true results in rpc auth failures openshift-metal3/dev-scripts#1099

Closed

zaneb mentioned this pull request Oct 27, 2020

Basic auth on json-rpc broken with IPv6 #210

Closed

		@@ -1,5 +1,10 @@
		[DEFAULT]
		{% if "HTTP_BASIC_HTPASSWD" in env %}

Switch ironic configuration rendering to Jinja2 #201

Switch ironic configuration rendering to Jinja2 #201

Conversation

maelk commented Sep 20, 2020 • edited Loading

maelk commented Sep 20, 2020

maelk commented Sep 20, 2020

maelk commented Sep 21, 2020

maelk commented Sep 21, 2020

maelk commented Sep 21, 2020

maelk commented Sep 21, 2020

dhellmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maelk commented Sep 21, 2020

dhellmann commented Sep 21, 2020

zaneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maelk commented Sep 22, 2020

maelk commented Sep 22, 2020

maelk commented Sep 24, 2020

dhellmann commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maelk commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maelk commented Sep 26, 2020

derekhiggins commented Sep 28, 2020

metal3-io-bot commented Sep 28, 2020

maelk commented Sep 28, 2020

zaneb commented Sep 28, 2020

maelk commented Sep 29, 2020

derekhiggins commented Sep 29, 2020

maelk commented Sep 20, 2020 •

edited

Loading