internal/*: drop merging authorized_keys.d into authorized_keys by rfairley · Pull Request #751 · coreos/ignition

rfairley · 2019-02-27T22:25:47Z

Will rebase this PR once #749 is merged.

So far I have verified that the blackbox test works, and that the old blackbox test fails with the build flag
GLDFLAGS+="-X github.com/coreos/ignition/internal/distro.useAuthorizedKeysFile=true " (in ./build_blackbox_tests).

Still to do:

Verify in a built OS image that this works (with/without useAuthorizedKeysFile=true)
Add documentation mentioned in Drop merging of authorized_keys.d into authorized_keys #716
Thinking of adding a blackbox test with the distro.useAuthorizedKeysFile=true flag that checks that authorized_keys gets updated

Fixes: #716

coreosbot · 2019-02-27T22:25:49Z

Can one of the admins verify this patch?

rfairley · 2019-03-04T15:18:17Z

Thinking of adding a blackbox test with the distro.useAuthorizedKeysFile=true flag that checks that authorized_keys gets updated

Hmm - finding it a bit tricky to add a test for this as writing to authorized_keys requires the linker flag setting which would need to be done in ./build_blackbox_tests. I wonder about setting distro.useAuthorizedKeysFile to "true" at runtime before executing a blackbox test, but distro.useAuthorizedKeysFile needs to be exported for that.

I think since Ignition v3 will support authorized_keys.d over authorized_keys (primarily), it should be sufficient having the blackbox test here which has been adjusted to check for the fragment in authorized_keys.d.

rfairley · 2019-03-04T15:20:29Z

Ready for a review

bgilbert

I think we should delete the authorized_keys_d code entirely. Ignition should be running before anything else has written keys to disk, so we should be able to completely clobber .ssh/authorized_keys if usedAuthorizedKeysFile is true.

bgilbert · 2019-03-04T16:37:30Z

For the blackbox tests we could expose the option via an environment variable, the same as for systemConfigDir.

rfairley · 2019-03-04T22:58:38Z

Made some edits:

Dropped the code in authorized_keys_d and brought as_user to the directory level above (I think the helper functions in as_user will still be needed, for writing the keyfile with the right uid/gid).
Made it so that if .ssh/authorized_keys is written, .ssh/authorized_keys.d/ignition is not. Would this behavior be preferred for distros that don't support authorized_keys.d? Thinking if merging doesn't need to be performed, then we can avoid having authorized_keys.d appear altogether.
Made distro.useAuthorizedKeysFile configurable with an environment variable. I did wonder about having this option be a path instead, i.e. distro.authorizedKeysFile. I can't think of any use case where any path other than authorized_key or authorized_keys.d/ignition are needed, but having it a path would simplify the logic in internal/exec/util/passwd.go (could just pass the given path into writeAuthKeysFile rather than have a conditional).

To do:

Add a blackbox test that uses IGNITION_USE_AUTHORIZED_KEYS_FILE=true. Will look into this tomorrow.

This allows individual tests to configure environment variables by setting the IgnitionEnv member of types.Test. The TestIgnitionBlackBox and TestIgnitionBlackBoxNegative functions may then append additional environment variables during setup that apply to all tests. Preparatory commit for adding a blackbox test in coreos#751.

rfairley · 2019-03-05T20:01:29Z

⬆ Pushed changes to add another blackbox test for .ssh/authorized_keys when IGNITION_USE_AUTHORIZED_KEYS_FILE=true. This adds IgnitionEnv as a member to types.Test. Also tidies up passwd.go a bit since removing authorized_keys_d.

Diff since the push yesterday: https://github.com/coreos/ignition/compare/6a6b24d5fffa0a957672c641a2361e730da11109..46f711b002e2ee7d988ccab6bac9ef1572477bef.

arithx · 2019-03-05T20:14:53Z

ok to test

rfairley · 2019-03-05T21:01:56Z

Lifted WIP - ready for another look.

For the distro-specific documentation part, should we handle that later in #649? (I left a comment there - so we won't forget). Or, I could get a doc started here in this PR, which #649 will add to/revise.

bgilbert · 2019-03-06T16:51:40Z

I'm okay handling the additional docs in #649.

bgilbert

LGTM generally.

This allows individual tests to configure environment variables by setting the IgnitionEnv member of types.Test. The TestIgnitionBlackBox and TestIgnitionBlackBoxNegative functions may then append additional environment variables during setup that apply to all tests. Preparatory commit for adding a blackbox test in coreos#751.

This allows individual tests to configure environment variables by setting the Env member of types.Test. The TestIgnitionBlackBox and TestIgnitionBlackBoxNegative functions may then append additional environment variables during setup that apply to all tests. Preparatory commit for adding a blackbox test in coreos#751.

ajeddeloh

LGTM with the changes @bgilbert suggested. We typically put tests in their own commit.

This allows individual tests to configure environment variables by setting the Env member of types.Test. The TestIgnitionBlackBox and TestIgnitionBlackBoxNegative functions may then append additional environment variables during setup that apply to all tests. Preparatory commit for adding a blackbox test in coreos#751.

rfairley · 2019-03-06T19:01:43Z

Updated! I split the added test into its own commit - which also updates the existing AddPasswdUsers test to set the environment variable in the same commit.

ajeddeloh

LGTM

rfairley · 2019-03-06T19:10:55Z

Hmm - failure just seems to be from the gofmt. Thinking it wants to indent the function due to the line length. Any way to disable/configure gofmt to not consider that a failure? Having the one-line function body is neater. https://travis-ci.org/coreos/ignition/jobs/502719646#L603

ajeddeloh · 2019-03-06T19:12:01Z

I'd say go with gofmt in this case, it is quite a long line otherwise.

This allows individual tests to configure environment variables by setting the Env member of types.Test. The TestIgnitionBlackBox and TestIgnitionBlackBoxNegative functions may then append additional environment variables during setup that apply to all tests. Preparatory commit for adding a blackbox test in coreos#751.

This drops the authorized_keys_d code, having Ignition directly write SSH keys to a keyfile. By default, Ignition writes the fragment to .ssh/authorized_keys.d/ignition. Setting the flag distro.writeAuthorizedKeysFragment to "false" (through the environment variable IGNITION_WRITE_AUTHORIZED_KEYS_FRAGMENT) causes Ignition to write the SSH keys to .ssh/authorized_keys. Distributions that do not read fragments from .ssh/authorized_keys.d can then instead read from .ssh/authorized_keys. Fixes: coreos#716

This allows individual tests to configure environment variables by setting the Env member of types.Test. The TestIgnitionBlackBox and TestIgnitionBlackBoxNegative functions may then append additional environment variables during setup that apply to all tests. Preparatory commit for adding a blackbox test in coreos#751.

Add a blackbox test to verify that ~/.ssh/authorized_keys is written when IGNITION_WRITE_AUTHORIZED_KEYS_FRAGMENT=false. Also updates the existing test AddPasswdUsers to use the environment variable IGNITION_WRITE_AUTHORIZED_KEYS_FRAGMENT=true so that the test will pass consistently if the binary is built with a non-default writeAuthorizedKeysFragment flag.

rfairley · 2019-03-06T19:18:25Z

Fixed! (had committed but forgot to squash it into 6b434dd just then). Should be good now.

ajeddeloh

LGTM

bgilbert · 2019-03-07T21:22:58Z

@rfairley Good to merge?

rfairley · 2019-03-07T21:28:52Z

@bgilbert Good to merge!

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 594b5ff to 0fba9d2 Compare March 4, 2019 14:35

rfairley changed the title ~~[WIP] internal/*: drop merging authorized_keys.d into authorized_keys~~ internal/*: drop merging authorized_keys.d into authorized_keys Mar 4, 2019

bgilbert reviewed Mar 4, 2019

View reviewed changes

Comment thread doc/configuration-v3_0-experimental.md Outdated

rfairley mentioned this pull request Mar 4, 2019

There are no docs for how to incorporate Ignition in a distro #649

Open

rfairley changed the title ~~internal/*: drop merging authorized_keys.d into authorized_keys~~ [WIP]: internal/*: drop merging authorized_keys.d into authorized_keys Mar 4, 2019

rfairley changed the title ~~[WIP]: internal/*: drop merging authorized_keys.d into authorized_keys~~ [WIP] internal/*: drop merging authorized_keys.d into authorized_keys Mar 4, 2019

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch 5 times, most recently from 0d927a0 to 6a6b24d Compare March 4, 2019 22:44

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 6a6b24d to 1abddeb Compare March 5, 2019 19:08

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 1abddeb to 2a2461e Compare March 5, 2019 19:33

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch 3 times, most recently from a0f3a1d to 46f711b Compare March 5, 2019 19:45

rfairley changed the title ~~[WIP] internal/*: drop merging authorized_keys.d into authorized_keys~~ internal/*: drop merging authorized_keys.d into authorized_keys Mar 5, 2019

rfairley changed the title ~~internal/*: drop merging authorized_keys.d into authorized_keys~~ [[WIP] internal/*: drop merging authorized_keys.d into authorized_keys Mar 5, 2019

rfairley changed the title ~~[[WIP] internal/*: drop merging authorized_keys.d into authorized_keys~~ [WIP] internal/*: drop merging authorized_keys.d into authorized_keys Mar 5, 2019

rfairley changed the title ~~[WIP] internal/*: drop merging authorized_keys.d into authorized_keys~~ internal/*: drop merging authorized_keys.d into authorized_keys Mar 5, 2019

bgilbert reviewed Mar 6, 2019

View reviewed changes

Comment thread tests/types/types.go Outdated

Comment thread internal/distro/distro.go Outdated

Comment thread internal/distro/distro.go Outdated

Comment thread tests/positive/passwd/users.go

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 46f711b to 4e06881 Compare March 6, 2019 18:18

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 4e06881 to 166a00c Compare March 6, 2019 18:20

ajeddeloh reviewed Mar 6, 2019

View reviewed changes

Comment thread tests/positive/passwd/users.go Outdated

Comment thread tests/positive/passwd/users.go Outdated

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 166a00c to 61eb130 Compare March 6, 2019 18:52

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 61eb130 to 57e19f7 Compare March 6, 2019 18:58

ajeddeloh approved these changes Mar 6, 2019

View reviewed changes

bgilbert approved these changes Mar 6, 2019

View reviewed changes

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 57e19f7 to 0460eb0 Compare March 6, 2019 19:15

Robert Fairley added 3 commits March 6, 2019 14:16

rfairley force-pushed the rfairley-drop-merging-authorized_keys_d branch from 0460eb0 to f89ffe2 Compare March 6, 2019 19:16

ajeddeloh approved these changes Mar 6, 2019

View reviewed changes

bgilbert approved these changes Mar 6, 2019

View reviewed changes

bgilbert merged commit 6d56258 into coreos:master Mar 7, 2019

rfairley deleted the rfairley-drop-merging-authorized_keys_d branch March 7, 2019 21:38

vrutkovs mentioned this pull request Feb 16, 2021

openshift-machine-config-operator: remove autopromotion to origin openshift/release#15814

Closed

Conversation

rfairley commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreosbot commented Feb 27, 2019

Uh oh!

rfairley commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfairley commented Mar 4, 2019

Uh oh!

bgilbert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bgilbert commented Mar 4, 2019

Uh oh!

rfairley commented Mar 4, 2019

Uh oh!

rfairley commented Mar 5, 2019

Uh oh!

arithx commented Mar 5, 2019

Uh oh!

rfairley commented Mar 5, 2019

Uh oh!

bgilbert commented Mar 6, 2019

Uh oh!

bgilbert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajeddeloh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rfairley commented Mar 6, 2019

Uh oh!

ajeddeloh left a comment

Choose a reason for hiding this comment

Uh oh!

rfairley commented Mar 6, 2019

Uh oh!

ajeddeloh commented Mar 6, 2019

Uh oh!

rfairley commented Mar 6, 2019

Uh oh!

ajeddeloh left a comment

Choose a reason for hiding this comment

Uh oh!

bgilbert commented Mar 7, 2019

Uh oh!

rfairley commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rfairley commented Feb 27, 2019 •

edited

Loading

rfairley commented Mar 4, 2019 •

edited

Loading