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

Use secrets for node-passwd entries #2407

Merged
merged 2 commits into from
Dec 8, 2020

Conversation

erikwilson
Copy link
Contributor

Proposed Changes

Store node passwords as hashed secrets and remove on node delete.

Types of Changes

Bugfix

Verification

Linked Issues

For #802

Further Comments

pkg/nodepassword/nodepassword.go Outdated Show resolved Hide resolved
pkg/nodepassword/nodepassword.go Outdated Show resolved Hide resolved
pkg/nodepassword/nodepassword.go Outdated Show resolved Hide resolved
pkg/nodepassword/nodepassword.go Outdated Show resolved Hide resolved
pkg/nodepassword/nodepassword.go Outdated Show resolved Hide resolved
@brandond
Copy link
Contributor

brandond commented Oct 21, 2020

Can I ask why scrypt? It's intentionally memory-intensive to make ASIC-based cracking hard; with N=2^15 each password comparison will require allocating about 32MB of memory. Most current guidelines suggest use of argon2.

@erikwilson
Copy link
Contributor Author

scrypt was used here because it utilizes pbkdf2 and sha256 under the hood, and has been used for awhile in rancher/rancher for cluster auth tokens, and is probably an upgrade compared to the bcrypt hashing otherwise used for authentication there.

Is the memory or cpu requirement in practice actually an issue? Which guidelines recommend using argon2? Is argon2 FIPS approved?

@brandond
Copy link
Contributor

Yeah I was curious about the fips question myself. Memory seems like a relevant consideration for edge use cases, but if scrypt is already vetted for use in rke2 then maybe we could just tune it for use in k3s?

@erikwilson
Copy link
Contributor Author

It feels like premature optimization, I don't think anything needs to be tuned (yet), but I suppose we could take some time testing on whatever relevant device.

@erikwilson erikwilson force-pushed the node-passwd-cleanup branch 4 times, most recently from 33b85b4 to ee5c65e Compare October 26, 2020 17:45
@erikwilson erikwilson changed the title WIP - Node password cleanup Use secrets for node-passwd entries Oct 26, 2020
@erikwilson erikwilson marked this pull request as ready for review October 26, 2020 18:26
@erikwilson erikwilson requested a review from a team as a code owner October 26, 2020 18:26
@erikwilson
Copy link
Contributor Author

Testing on x86 hashing is about ~100ms each, armv6 w/ rpi4 is ~600ms. As a non-interactive verification which should be infrequent this is probably acceptable. Memory usage should be fairly negligible assuming serial hashing as the memory is collected & re-used. Should be fairly easy to limit parallel hashing or lower scrypt N value if need as a future update.

@erikwilson erikwilson requested a review from a team October 30, 2020 19:26
coreclient "github.com/rancher/wrangler-api/pkg/generated/controllers/core/v1"
"github.com/sirupsen/logrus"
core "k8s.io/api/core/v1"
)

func Register(ctx context.Context, configMap coreclient.ConfigMapController, nodes coreclient.NodeController) error {
func Register(ctx context.Context,
Copy link
Contributor

Choose a reason for hiding this comment

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

pkg/node/controller.go Outdated Show resolved Hide resolved
@@ -31,3 +31,5 @@ if [ -n "$DIRTY" ]; then
git status --porcelain --untracked-files=no
exit 1
fi

"${GO}" test -v ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants