WINC-632: Secure BYOH username annotation #508

saifshaikh48 · 2021-07-08T16:53:40Z

This PR introduces symmetric encryption and decryption into the usage of the instance username annotation to secure potentially sensitive user info. When a user changes the private key used to SSH into the instances, each BYOH node's annotation is updated to be encrypted with the new key.

saifshaikh48 · 2021-07-14T16:52:19Z

/retitle WINC-632: Secure BYOH username annotation

saifshaikh48 · 2021-07-14T16:52:57Z

/hold

Holding until CI (vsphere) is fixed.

saifshaikh48 · 2021-07-16T16:25:04Z

This PR is blocked by #518, which fixes an error when applying node annotations.

aravindhp · 2021-07-16T16:31:27Z

Why is this PR blocked by #518? Doesn't the Node object get updated on the second try with Update()? At least that is what I have been observing.

saifshaikh48 · 2021-07-16T16:59:42Z

Why is this PR blocked by #518? Doesn't the Node object get updated on the second try with Update()? At least that is what I have been observing.

You're right, badly worded on my end -- #518 should ideally merge first.

saifshaikh48 · 2021-07-19T16:14:09Z

/unhold
/test ci/prow/ci-index

openshift-ci · 2021-07-19T16:14:19Z

@saifshaikh48: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test aws-e2e-operator
/test aws-e2e-upgrade
/test azure-e2e-operator
/test build
/test ci-index
/test images
/test lint
/test unit
/test vsphere-e2e-operator

Use /test all to run all jobs.

In response to this:

/unhold
/test ci/prow/ci-index

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

saifshaikh48 · 2021-07-19T16:14:51Z

/test ci-index

saifshaikh48 · 2021-07-19T18:17:17Z

controllers/secret_controller.go

+				// For BYOH nodes, patch the username annotation, encrpyting with the new private key
+				username, err := r.usernameFromNode(ctx, &node)
+				if err != nil {
+					return reconcile.Result{}, err
+				}


Reviewing the logic here -- could I get another pair of eyes:

If usernameFromNode finds no associated instance then that means its entry has been removed from the ConfigMap, and the instance should be deconfigured (or is currently being deconfigured, since the configmap controller can be running in parallel). Therefore, this isn't an error scenario and should return reconcile.Result{}, nil.

I think we still want an err check so maybe you could instead add in the empty instance check that returns reconcile.Result{}, nil

aravindhp

Nice work @saifshaikh48

/approve
/hold

Adding a hold that @sebsoto can remove given I did not review the e2e tests very closely.

openshift-ci · 2021-07-30T21:30:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp, saifshaikh48

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~~ [aravindhp]

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

sebsoto · 2021-08-02T14:31:29Z

test/e2e/secrets_test.go

+	// Re-create the known private key so SSH connection can be re-established
+	// TODO: Remove dependency on this secret by rotating keys as part of https://issues.redhat.com/browse/WINC-655
+	require.NoError(t, tc.createPrivateKeySecret(true), "error confirming known private key secret exists")


I'm still on the fence about this but it seems to be working...
This operation is going to bleed into the deletion suite as the WMCO will be handling this first and could taint the results that we expect when doing certain deletion actions. But what we are testing for in the BYOH reconfiguration, specifically the removal of binaries and services, shouldnt be triggered by this.

I'm fine with this going in but we need to handle the TODO story ASAP so that we dont open ourselves up to missing certain edge cases

sebsoto · 2021-08-02T14:33:05Z

/lgtm

sebsoto · 2021-08-02T14:33:17Z

/hold cancel

openshift-bot · 2021-08-02T14:41:48Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-08-02T15:08:05Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

sebsoto · 2021-08-02T18:47:19Z

/hold

sebsoto · 2021-08-02T18:53:45Z

/hold cancel

mansikulkarni96 · 2021-08-02T19:09:33Z

/test vsphere-e2e-operator

openshift-bot · 2021-08-02T20:31:59Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

mansikulkarni96 · 2021-08-02T21:02:59Z

/cherry-pick community-4.8

mansikulkarni96 · 2021-08-02T21:03:13Z

/cherry-pick release-4.8

openshift-cherrypick-robot · 2021-08-02T21:03:35Z

@mansikulkarni96: #508 failed to apply on top of branch "community-4.8":

Applying: Remove exposing username in logs
Applying: Secure username anno with cloud private key
Using index info to reconstruct a base tree...
M	controllers/configmap_controller.go
M	controllers/controllers.go
Falling back to patching base and 3-way merge...
Auto-merging controllers/controllers.go
Auto-merging controllers/configmap_controller.go
CONFLICT (content): Merge conflict in controllers/configmap_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Secure username anno with cloud private key
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick community-4.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2021-08-02T21:03:47Z

@mansikulkarni96: #508 failed to apply on top of branch "release-4.8":

Applying: Remove exposing username in logs
Applying: Secure username anno with cloud private key
Using index info to reconstruct a base tree...
M	controllers/configmap_controller.go
M	controllers/controllers.go
Falling back to patching base and 3-way merge...
Auto-merging controllers/controllers.go
CONFLICT (content): Merge conflict in controllers/controllers.go
Auto-merging controllers/configmap_controller.go
CONFLICT (content): Merge conflict in controllers/configmap_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Secure username anno with cloud private key
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sebsoto · 2021-08-04T11:36:52Z

/cherry-pick community-4.8

openshift-cherrypick-robot · 2021-08-04T11:37:29Z

@sebsoto: new pull request created: #560

In response to this:

/cherry-pick community-4.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mansikulkarni96 · 2021-08-16T16:10:36Z

/cherry-pick release-4.8

mansikulkarni96 · 2021-08-16T16:15:07Z

/cherry-pick release-4.8

openshift-cherrypick-robot · 2021-08-16T16:15:46Z

@mansikulkarni96: new pull request created: #604

In response to this:

/cherry-pick release-4.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2021

openshift-ci bot requested review from aravindhp and sebsoto July 8, 2021 16:53

saifshaikh48 force-pushed the encrypt-username-annotation branch from 0a22845 to f01de8f Compare July 8, 2021 20:37

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2021

saifshaikh48 force-pushed the encrypt-username-annotation branch 2 times, most recently from 34718d7 to 1b6a36b Compare July 14, 2021 15:13

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021

openshift-ci bot changed the title ~~WIP: WINC-632: Secure BYOH username annotation~~ WINC-632: Secure BYOH username annotation Jul 14, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2021

saifshaikh48 force-pushed the encrypt-username-annotation branch from 1b6a36b to 63b7af4 Compare July 16, 2021 16:21

saifshaikh48 force-pushed the encrypt-username-annotation branch from 63b7af4 to 9ba07c2 Compare July 16, 2021 21:40

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2021

saifshaikh48 force-pushed the encrypt-username-annotation branch 2 times, most recently from a938eb5 to a423e5c Compare July 19, 2021 15:33

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2021

saifshaikh48 commented Jul 19, 2021

View reviewed changes

saifshaikh48 force-pushed the encrypt-username-annotation branch 3 times, most recently from 811a854 to 4d76992 Compare July 21, 2021 22:29

aravindhp approved these changes Jul 30, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2021

sebsoto reviewed Aug 2, 2021

View reviewed changes

openshift-ci bot assigned sebsoto Aug 2, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2021

openshift-ci bot merged commit 445c027 into openshift:master Aug 2, 2021

openshift-cherrypick-robot mentioned this pull request Aug 4, 2021

[community-4.8] WINC-632: Secure BYOH username annotation #560

Merged

openshift-cherrypick-robot mentioned this pull request Aug 16, 2021

[release-4.8] WINC-632: Secure BYOH username annotation #604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WINC-632: Secure BYOH username annotation #508

WINC-632: Secure BYOH username annotation #508

saifshaikh48 commented Jul 8, 2021 •

edited

saifshaikh48 commented Jul 14, 2021 •

edited by openshift-ci bot

saifshaikh48 commented Jul 14, 2021

saifshaikh48 commented Jul 16, 2021

aravindhp commented Jul 16, 2021

saifshaikh48 commented Jul 16, 2021

saifshaikh48 commented Jul 19, 2021

openshift-ci bot commented Jul 19, 2021

saifshaikh48 commented Jul 19, 2021

saifshaikh48 Jul 19, 2021

alinaryan Jul 19, 2021

aravindhp left a comment

openshift-ci bot commented Jul 30, 2021

sebsoto Aug 2, 2021

sebsoto commented Aug 2, 2021

sebsoto commented Aug 2, 2021

openshift-bot commented Aug 2, 2021

openshift-bot commented Aug 2, 2021

sebsoto commented Aug 2, 2021

sebsoto commented Aug 2, 2021

mansikulkarni96 commented Aug 2, 2021

openshift-bot commented Aug 2, 2021

mansikulkarni96 commented Aug 2, 2021

mansikulkarni96 commented Aug 2, 2021

openshift-cherrypick-robot commented Aug 2, 2021

openshift-cherrypick-robot commented Aug 2, 2021

sebsoto commented Aug 4, 2021

openshift-cherrypick-robot commented Aug 4, 2021

mansikulkarni96 commented Aug 16, 2021

mansikulkarni96 commented Aug 16, 2021

openshift-cherrypick-robot commented Aug 16, 2021

WINC-632: Secure BYOH username annotation #508

WINC-632: Secure BYOH username annotation #508

Conversation

saifshaikh48 commented Jul 8, 2021 • edited

saifshaikh48 commented Jul 14, 2021 • edited by openshift-ci bot

saifshaikh48 commented Jul 14, 2021

saifshaikh48 commented Jul 16, 2021

aravindhp commented Jul 16, 2021

saifshaikh48 commented Jul 16, 2021

saifshaikh48 commented Jul 19, 2021

openshift-ci bot commented Jul 19, 2021

saifshaikh48 commented Jul 19, 2021

saifshaikh48 Jul 19, 2021

Choose a reason for hiding this comment

alinaryan Jul 19, 2021

Choose a reason for hiding this comment

aravindhp left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 30, 2021

sebsoto Aug 2, 2021

Choose a reason for hiding this comment

sebsoto commented Aug 2, 2021

sebsoto commented Aug 2, 2021

openshift-bot commented Aug 2, 2021

openshift-bot commented Aug 2, 2021

sebsoto commented Aug 2, 2021

sebsoto commented Aug 2, 2021

mansikulkarni96 commented Aug 2, 2021

openshift-bot commented Aug 2, 2021

mansikulkarni96 commented Aug 2, 2021

mansikulkarni96 commented Aug 2, 2021

openshift-cherrypick-robot commented Aug 2, 2021

openshift-cherrypick-robot commented Aug 2, 2021

sebsoto commented Aug 4, 2021

openshift-cherrypick-robot commented Aug 4, 2021

mansikulkarni96 commented Aug 16, 2021

mansikulkarni96 commented Aug 16, 2021

openshift-cherrypick-robot commented Aug 16, 2021

saifshaikh48 commented Jul 8, 2021 •

edited

saifshaikh48 commented Jul 14, 2021 •

edited by openshift-ci bot