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

Limit ArgoCD password seed to the max length bcrypt allows #137

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

bastjan
Copy link
Contributor

@bastjan bastjan commented Nov 14, 2023

By design, bcrypt only uses the first 72 bytes of a password when
generating a hash. Most implementations, including the reference one,
simply silently ignore any trailing input when provided passwords longer
than 72 bytes. This can cause confusion for users who expect the entire
password to be used to generate the hash.

https://cs.opensource.google/go/x/crypto/+/bc7d1d1eb54b3530da4f5ec31625c95d7df40231

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Update the documentation.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog

@bastjan bastjan added the bug Something isn't working label Nov 14, 2023
@bastjan bastjan requested a review from a team November 14, 2023 17:24
@bastjan bastjan force-pushed the fix-pw-too-long-error branch from 146659b to 1eef39d Compare November 15, 2023 07:47
@bastjan bastjan merged commit 6ef601b into master Nov 15, 2023
3 checks passed
@bastjan bastjan deleted the fix-pw-too-long-error branch November 15, 2023 07:56
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

Please test this thoroughly, we currently use the base64-encoded Lieutenant JWT which is definitely longer than 72 characters as the password, cf.

if err := argocd.CreateArgoSecret(ctx, config, a.Namespace, a.Token); err != nil {

I'm not sure if we assume that the password will be the full JWT elsewhere.

@bastjan
Copy link
Contributor Author

bastjan commented Nov 15, 2023

I'm not sure if we assume that the password will be the full JWT elsewhere.

The remainder of the password was always ignored:

go.mod

require (
	golang.org/x/crypto v0.15.0
	local.dev/oldcrypto v0.0.0-00010101000000-000000000000
)

replace local.dev/oldcrypto => golang.org/x/crypto v0.0.0-20220525230936-793ad666bf5e

bcrypt_test.go

package testbcrypt_test

import (
	"strings"
	"testing"

	"github.com/stretchr/testify/require"
	"golang.org/x/crypto/bcrypt"
	old "local.dev/oldcrypto/bcrypt"
)

func TestBcrypt(t *testing.T) {
	pw := strings.Repeat("a", 100)
	shortPw := strings.Repeat("a", 72)

	oldEnc, err := old.GenerateFromPassword([]byte(pw), old.DefaultCost)
	require.NoError(t, err)

	enc, err := bcrypt.GenerateFromPassword([]byte(shortPw), bcrypt.DefaultCost)
	require.NoError(t, err)

	require.NoError(t, old.CompareHashAndPassword(enc, []byte(shortPw)))
	require.NoError(t, old.CompareHashAndPassword(enc, []byte(pw)))
	require.NoError(t, old.CompareHashAndPassword(oldEnc, []byte(shortPw)))
	require.NoError(t, old.CompareHashAndPassword(oldEnc, []byte(pw)))
	require.NoError(t, bcrypt.CompareHashAndPassword(enc, []byte(shortPw)))
	require.NoError(t, bcrypt.CompareHashAndPassword(enc, []byte(pw)))
	require.NoError(t, bcrypt.CompareHashAndPassword(oldEnc, []byte(shortPw)))
	require.NoError(t, bcrypt.CompareHashAndPassword(oldEnc, []byte(pw)))
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants