Skip to content

Add ability to use existing PSK secrets #32

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

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

pranavgaikwad
Copy link
Contributor

@pranavgaikwad pranavgaikwad commented Aug 5, 2022

Signed-off-by: Pranav Gaikwad pgaikwad@redhat.com

Describe what this PR does

  • Introduces new field in Transport Options to specify existing secrets:
    • Secrets may either contain TLS certs or a Pre-Shared Key
    • New secret is not created if existing secret is valid
  • Updates rsync termination logic:
    • Rsync client will now send a file to a known location on the server to indicate transfer completion, server will exit when the file is present in destination. Previously, _exit message was read from server logs to exit
    • Transport container will exit when underlying Transfer is gone

Is there anything that requires special attention?
~This PR does NOT add support for creating a new PSK when existing is invalid. Creating PSK is not supported in go currently [1]. Users will need to provide their existing PSK in a secret. If not provided, library will default to creating new TLS certs instead. ~

Updated to create a 32 byte long PSK key. Stunnel requires key to be base64 encoded.

Related issues:
[1] golang/go#6379

Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pranavgaikwad

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:

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

@openshift-ci openshift-ci bot added the approved label Aug 5, 2022
@pranavgaikwad
Copy link
Contributor Author

/cc @JohnStrunk

@openshift-ci openshift-ci bot requested a review from JohnStrunk August 5, 2022 16:42
@openshift-ci openshift-ci bot added the size/XL label Aug 5, 2022
@JohnStrunk
Copy link
Member

For generating the PSK, we should be able to just use:

func GeneratePassword() (string, error) {
var letters = []byte("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")
password := make([]byte, 24)
for i := 0; i < 24; i++ {
num, err := rand.Int(rand.Reader, big.NewInt(int64(len(letters))))
if err != nil {
return "", err
}
password = append(password, letters[num.Int64()])
}
return string(password), nil
}

This creates a ~128bit key:

  • Each character provides: ln(26*2)/ln(2) bits of entropy = 5.7
  • 5.7 * 24 characters = 136 bits

... looking at the rest of the PR now

PSKsecrets = /etc/stunnel/certs/key
{{ end }}

[transfer]
Copy link
Member

Choose a reason for hiding this comment

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

It looks like [transfer] got moved to below the key specification... was this intentional (and ok?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JohnStrunk Yes, the settings will apply to all subsections including the one used for termination. Its kind of a global setting across mutliple transfers

UseTLS: true,
}
if sc.options.Credentials != nil && sc.options.Credentials.Type == CredentialsTypePSK {
fields.UseTLS = false
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this UsePSK or something? It's still TLS, even w/ the PSK, it's just TLS-PSK instead of using public-key crypto.

Comment on lines +200 to +218
stunnelScript := `/bin/stunnel /etc/stunnel/stunnel.conf
# terminate the transport when transfer isn't available
RETRY=0
while true; do
nc -z localhost %d
rc=$?
if [ $rc -ne 0 ]; then
RETRY=$((RETRY+1))
else
RETRY=0
fi
if [ $RETRY -gt 10 ]; then
exit 0
else
sleep 1
fi
done
`
stunnelScript = fmt.Sprintf(stunnelScript, s.ConnectPort())
Copy link
Member

Choose a reason for hiding this comment

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

I'm not following the flow...
I assume this goes with the change to foreground = no. Is the logic here:

  • we're putting stunnel in the bg and having the script (main entrypoint) probe the connection's availability
  • When it's no longer available, the entrypoint terminates, bringing the container down and killing the background stunnel process since the container terminates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JohnStrunk That's correct.

@@ -290,3 +321,18 @@ func markForCleanup(ctx context.Context, c ctrlclient.Client, objKey types.Names

return nil
}

// GeneratePassword can be used to generate random character string for 24 byte
Copy link
Member

Choose a reason for hiding this comment

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

24 --> 32 bytes?

Copy link
Contributor Author

@pranavgaikwad pranavgaikwad Aug 9, 2022

Choose a reason for hiding this comment

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

@JohnStrunk yeah as per Stunnel documentation, stunnel allows either 16 or 32 bytes PSK. I didn't really try giving a 24 bytes value, I can check if its something we want to do.

Copy link
Member

Choose a reason for hiding this comment

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

No problem. The comment just says 24, but the implementation is 32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh okay.

Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
@JohnStrunk
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 10, 2022
@openshift-merge-robot openshift-merge-robot merged commit 5f9e29a into backube:main Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants