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

feat: PUC-752: reading OIDCCryptoPassphrase from a file #650

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

haseebsyed12
Copy link
Contributor

@haseebsyed12 haseebsyed12 commented Feb 5, 2025

Problem:
Unable to authenticate when the user receives the initial state, it is encrypted with a key from pod A, then user is redirected to the identity provided and finally when they get back through redirect, they may land on pod B which is no longer able to decrypt the state.

Solution:

OIDCCryptoPassphrase is a password used for encryption of the state cookie and cache entries and it must be the same on all replicas.

Previously we were setting the OIDCCryptoPassphrase to a random value that is generated on pod startup and multiple pods have different value.

@haseebsyed12 haseebsyed12 requested a review from a team February 5, 2025 00:55
Copy link
Contributor

@cardoe cardoe left a comment

Choose a reason for hiding this comment

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

Can you write a description of why the change is needed to leave us some bread crumbs for future logic. I've recently found myself needing to refer to our commits in upstream OpenStack projects and our commits have been lacking in explanation which would be helpful.

@haseebsyed12 haseebsyed12 force-pushed the puc-752_oidccryptopassphrase branch 3 times, most recently from 323ef12 to 19b5006 Compare February 6, 2025 13:57
@haseebsyed12 haseebsyed12 marked this pull request as ready for review February 6, 2025 14:24
@haseebsyed12 haseebsyed12 force-pushed the puc-752_oidccryptopassphrase branch from 52478cb to 2a78532 Compare February 7, 2025 08:43
@haseebsyed12 haseebsyed12 force-pushed the puc-752_oidccryptopassphrase branch from 2a78532 to 8b9d3fc Compare February 7, 2025 11:34
Comment on lines 266 to 274
default_pwgen() {
"${SCRIPTS_DIR}/pwgen.sh" 2>/dev/null
}

# Custom password generator with only alphabets
# shellcheck disable=SC2317
alpha_only_pwgen() {
head /dev/urandom | tr -dc A-Za-z | head -c 32
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default_pwgen() {
"${SCRIPTS_DIR}/pwgen.sh" 2>/dev/null
}
# Custom password generator with only alphabets
# shellcheck disable=SC2317
alpha_only_pwgen() {
head /dev/urandom | tr -dc A-Za-z | head -c 32
}
default_pwgen() {
"${SCRIPTS_DIR}/pwgen.sh" 2>/dev/null
}
# Custom password generator with only alphabets
# shellcheck disable=SC2317
alpha_only_pwgen() {
head /dev/urandom | tr -dc A-Za-z | head -c 32
}

So we're not setting LC_ALL to C like the pwgen.sh script does. Let's instead add another parameter to pwgen.sh and let you specify the range.

So in that script do tr -dc ${2:-_A-Z-a-z-0-9}

@@ -274,7 +286,7 @@ load_or_gen_os_secret() {
return 1
else
echo "Generating ${secret_var}"
data="$("${SCRIPTS_DIR}/pwgen.sh" 2>/dev/null)"
data="$(${gen_func})"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data="$(${gen_func})"
data="$("${SCRIPTS_DIR}/pwgen.sh" 32 "A-Za-z" 2>/dev/null)"

@haseebsyed12 haseebsyed12 requested a review from cardoe February 10, 2025 06:14
@haseebsyed12 haseebsyed12 force-pushed the puc-752_oidccryptopassphrase branch from 1e56cc6 to b29edcd Compare February 10, 2025 12:28
@cardoe cardoe added this pull request to the merge queue Feb 10, 2025
Merged via the queue into main with commit 31ede5a Feb 10, 2025
14 checks passed
@cardoe cardoe deleted the puc-752_oidccryptopassphrase branch February 10, 2025 14:45
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.

3 participants