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.

Sign up for GitHub

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

tsh: Fix redundant error in PPK generation on relogin #23899

Merged
merged 4 commits into from
Apr 3, 2023
Merged

tsh: Fix redundant error in PPK generation on relogin #23899

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a8d16b
tsh: Fix redundant error in PPK generation on relogin
webvictim Mar 31, 2023
ebd111d
Switch to error at debug level rather than stack trace
webvictim Mar 31, 2023
0e98d99
Merge branch 'master' into gus/fix-redundant-tsh-ppk-error
webvictim Mar 31, 2023
0dca0c3
Merge branch 'master' into gus/fix-redundant-tsh-ppk-error
webvictim Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions lib/client/keystore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,17 +160,19 @@ func (fs *FSKeyStore) AddKey(key *Key) error {
if err := fs.writeBytes(key.TLSCert, fs.tlsCertPath(key.KeyIndex)); err != nil {
return trace.Wrap(err)
}

// We only generate PPK files for use by PuTTY when running tsh on Windows.
if runtime.GOOS == constants.WindowsOS {
ppkFile, err := key.PPKFile()
if err == nil {
// PPKFile can only be generated from an RSA private key. If the key is in a different
// format, a BadParameter error is returned and we can skip PPK generation.
if err != nil && !trace.IsBadParameter(err) {
fs.log.WithError(err).Debugf("Cannot convert private key to PPK-formatted keypair.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure WithError will print a stacktrace, which looks pretty nefarious in logs, but this isn't a hard error and is logged at debug level.

Suggested change
fs.log.WithError(err).Debugf("Cannot convert private key to PPK-formatted keypair.")
fs.log.Debugf("Cannot convert private key to PPK-formatted keypair: %v", err)

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the expected behavior here if it is a bad parameter error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BadParameter is thrown when the key passed in cannot be parsed as rsa.PrivateKey:

teleport/api/utils/keys/privatekey.go

Lines 137 to 140 in 6973cd6

rsaKey, ok := k.Signer.(*rsa.PrivateKey)
if !ok {
return nil, trace.BadParameter("cannot use private key of type %T as rsa.PrivateKey", k)
}

We just ignore this key in that case and continue login as normal.

Philosophically, I've always treated failure to generate a PPK file in the first place as a non-error, because PuTTY support is pretty optional and it shouldn't block the functionality of tsh login. On the other hand, if we do successfully generate the PPK but then fail to write it to disk successfully, I think that's a different kind of error that's worth halting a login for.

} else {
if err := fs.writeBytes(ppkFile, fs.ppkFilePath(key.KeyIndex)); err != nil {
return trace.Wrap(err)
}
} else if !trace.IsBadParameter(err) {
return trace.Wrap(err)
}
// PPKFile can only be generated from an RSA private key.
fs.log.WithError(err).Debugf("Cannot convert private key to PPK-formatted keypair.")
}

// Store per-cluster key data.
Expand Down