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

kola: support for ssh RSA key types other than RSA-SHA1 #1772

Closed
dustymabe opened this issue Oct 6, 2020 · 18 comments · Fixed by #2760
Closed

kola: support for ssh RSA key types other than RSA-SHA1 #1772

dustymabe opened this issue Oct 6, 2020 · 18 comments · Fixed by #2760
Labels
jira for syncing to jira

Comments

@dustymabe
Copy link
Member

Basically we need to support a key type other than RSA-SHA1 so that kola can ssh in to F33+.

The short term plan is to only re-enable RSA-SHA1 briefly while we wait for an upstream feature [1] to be implemented. We had moved to an ecdsa key [2] but AWS doesn't support non RSA keys [3] so we reverted it for now in [4].

[1] golang/go#37278 (comment)
[2] #1749
[3] https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws
[4] #1767

Once the upstream golang issue is fixed let's pick up that work in mantle/kola.

@darkmuggle
Copy link
Contributor

@dustymabe isn't this closed with #1749?

@dustymabe
Copy link
Member Author

@dustymabe isn't this closed with #1749?

See #1767

@dustymabe
Copy link
Member Author

Temporarily worked around this in the configs. This is a very short term change. We don't want to have f33 go to testing or stable with it in place.

@dustymabe
Copy link
Member Author

dustymabe commented Oct 7, 2020

Benjamin pointed out that the changes in Fedora and eventually openssh don't actually mean we have to change the key we're generating, but rather the key exchange algorithm. In order to do that we need to update our ssh client (kola use of the ssh library in this case) to use the new preferred algorithms. It seems we do still want the code referenced in my comment at golang/go#37278 (comment) so we can update kola to do that. So this issue still stands and we'll monitor to see if that code lands this week or next week. If not, we'll switch to an ecdsa key and just disable the afterburn ssh key test for now.

cc @bgilbert for fact checking.

@bgilbert
Copy link
Contributor

bgilbert commented Oct 8, 2020

Right. We could disable the Afterburn test exclusively on AWS. It's possible that we might still need to pass AWS a fake RSA key to make the API happy.

dustymabe added a commit to dustymabe/coreos-assembler that referenced this issue Oct 20, 2020
In 5c036d1 we swtched to ecdsa keys for the new crypto
policy change. In fdcfbbc we switched it back because
when we create new ssh keys on AWS we'd get an error because
that platform only accepts rsa keys:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws

For now add both a rsa key and a ecdsa key.
Using an ecdsa key allows us to workaround the updated
crypto policies in F33+ that disallow the RSA SHA-1 keyexchange
algorithm, but we still need an ssh-rsa key because AWS only
supports ssh-rsa keys and a key gets generated and added to
AWS every time we start a kola run. We'll add the rsa key
first since platform/machine/aws/flight.go only adds the first
key (keys[0]). We should be able to go back to RSA keys only
when the golang library is updated. More info in:
coreos#1772
dustymabe added a commit to dustymabe/coreos-assembler that referenced this issue Oct 20, 2020
In 5c036d1 we swtched to ecdsa keys for the new crypto
policy change. In fdcfbbc we switched it back because
when we create new ssh keys on AWS we'd get an error because
that platform only accepts rsa keys:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws

For now add both a rsa key and a ecdsa key.
Using an ecdsa key allows us to workaround the updated
crypto policies in F33+ that disallow the RSA SHA-1 keyexchange
algorithm, but we still need an ssh-rsa key because AWS only
supports ssh-rsa keys and a key gets generated and added to
AWS every time we start a kola run. We'll add the rsa key
first since platform/machine/aws/flight.go only adds the first
key (keys[0]). We should be able to go back to RSA keys only
when the golang library is updated. More info in:
coreos#1772
dustymabe added a commit to dustymabe/coreos-assembler that referenced this issue Oct 20, 2020
In 5c036d1 we swtched to ecdsa keys for the new crypto
policy change. In fdcfbbc we switched it back because
when we create new ssh keys on AWS we'd get an error because
that platform only accepts rsa keys:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws

For now let's disable the `fcos.ignition.v3.noop` on AWS and
just use a fake key for that platform to satisfy the requirement.
Eventually we'll switch back to RSA keys once the golang library
has been updated.

See coreos#1772
dustymabe added a commit to dustymabe/coreos-assembler that referenced this issue Oct 20, 2020
In 5c036d1 we swtched to ecdsa keys for the new crypto
policy change. In fdcfbbc we switched it back because
when we create new ssh keys on AWS we'd get an error because
that platform only accepts rsa keys:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws

For now let's disable the `fcos.ignition.v3.noop` on AWS and
just use a fake key for that platform to satisfy the requirement.
Eventually we'll switch back to RSA keys once the golang library
has been updated.

See coreos#1772
dustymabe added a commit to dustymabe/coreos-assembler that referenced this issue Oct 20, 2020
In 5c036d1 we switched to ecdsa keys for the new crypto
policy change. In fdcfbbc we switched it back because
when we create new ssh keys on AWS we'd get an error because
that platform only accepts rsa keys:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws

For now let's disable the `fcos.ignition.v3.noop` on AWS, use
a fake key for that platform to satisfy the requirement, and use
an ECDSA key everywhere else. Eventually we'll switch back to
RSA keys once the golang library has been updated.

See coreos#1772
dustymabe added a commit to dustymabe/coreos-assembler that referenced this issue Oct 20, 2020
In 5c036d1 we switched to ecdsa keys for the new crypto
policy change. In fdcfbbc we switched it back because
when we create new ssh keys on AWS we'd get an error because
that platform only accepts rsa keys:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws

For now let's disable the `fcos.ignition.v3.noop` on AWS, use
a fake key for that platform to satisfy the requirement, and use
an ECDSA key on other clouds and in all Ignition configs. Eventually
we'll switch back to a single RSA key once the golang library
has been updated.

See coreos#1772
openshift-merge-robot pushed a commit that referenced this issue Oct 20, 2020
In 5c036d1 we switched to ecdsa keys for the new crypto
policy change. In fdcfbbc we switched it back because
when we create new ssh keys on AWS we'd get an error because
that platform only accepts rsa keys:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws

For now let's disable the `fcos.ignition.v3.noop` on AWS, use
a fake key for that platform to satisfy the requirement, and use
an ECDSA key on other clouds and in all Ignition configs. Eventually
we'll switch back to a single RSA key once the golang library
has been updated.

See #1772
@dustymabe
Copy link
Member Author

dustymabe commented Oct 22, 2020

ok we're working around this temporarily by using an ecdsa key for Ignition/other cloud providers and using a throwaway key for AWS, and also disabling the fcos.ignition.v3.noop test on AWS. We should be able to revert the "workaround" commit and go back to using a single rsa key when we update the golang libraries.

@travier
Copy link
Member

travier commented Aug 24, 2021

https://aws.amazon.com/about-aws/whats-new/2021/08/amazon-ec2-customers-ed25519-keys-authentication/

@bgilbert
Copy link
Contributor

Excitingly, Azure doesn't support Ed25519.

@dustymabe
Copy link
Member Author

Excitingly, Azure doesn't support Ed25519.

I wonder if there is an issue somewhere where we could follow updates on that.

Anywho, we should be able to just switch on type of key the way we currently do (see ac54a8a) and re-enable the fcos.ignition.misc.empty and fcos.ignition.v3.noop tests for AWS.

@dustymabe dustymabe added the jira for syncing to jira label Oct 19, 2021
@bgilbert
Copy link
Contributor

Anywho, we should be able to just switch on type of key the way we currently do (see ac54a8a) and re-enable the fcos.ignition.misc.empty and fcos.ignition.v3.noop tests for AWS.

To be clear, are you proposing generating different key types for different platforms? There's no non-RSA key type that's supported by every platform.

@dustymabe
Copy link
Member Author

dustymabe commented Oct 19, 2021

To be clear, are you proposing generating different key types for different platforms? There's no non-RSA key type that's supported by every platform.

we can just use EDCSA in most places (what we're doing now) and then switch AWS to Ed25519 instead of RSA, which is what we're currently doing:

// We have worked around the golang library limitation for
// keyexchange algorithm by switching to an ecdsa key in
// network/ssh.go. However, AWS requires an rsa key. For now
// (until we get an updated golang library) we'll just satisfy
// the requirement by using a fake key and disabling the
// fcos.ignition.misc.empty and fcos.ignition.v3.noop tests on AWS.
// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-key-pairs.html#how-to-generate-your-own-key-and-import-it-to-aws
key, err := platform.GenerateFakeKey()
if err != nil {
af.Destroy()
return nil, err
}
if err := api.AddKey(af.Name(), key); err != nil {
af.Destroy()
return nil, err
}
af.keyAdded = true

Maybe there's something I'm missing?

@bgilbert
Copy link
Contributor

Again, just to be clear, your proposal is to plumb a platform-specific key type into the code that generates the real keys? The thing that's confusing me is that the implication that we're doing something similar now, which we are not. The snippet you quoted generates a non-operable key for AWS.

@dustymabe
Copy link
Member Author

Indeed. As is often the case things are more complicated than they seem. I saw that we were generating an RSA key for AWS only and assumed we could just do the same thing but make it an ed25519 key instead. I think what was confusing to me is that this key isn't usable for reasons other than it being an RSA key and our crypto policies not allowing it because of the golang library unimplented feature.

I think you're implying to make it usable we'd need to update other parts of the code.

@bgilbert
Copy link
Contributor

Right, we're generating an unused RSA key there only because the API requires that we provide something.

I do think we could plumb a key type argument into the key generation code; the call stack is NewFlight -> NewBaseFlight -> NewBaseFlightWithDialer -> NewSSHAgent. In principle we could be hacky and have NewBaseFlightWithDialer condition on the existing platform-name argument, but I think it's not too much more work to be explicit.

It doesn't look like there's been much movement on golang/go#37278.

@bgilbert
Copy link
Contributor

bgilbert commented Nov 9, 2021

It turns out that Azure doesn't support ECDSA either, so kola currently can't run tests on Azure (#2445).

@bgilbert
Copy link
Contributor

golang/go#37278 was fixed upstream, but it appears to be scoped to SSH host keys and not pubkey authentication. I think we need golang/go#39885 for that.

@jmarrero
Copy link
Member

@bgilbert does that mean that we are still blocked on this until golang/go#39885 is done?

@bgilbert
Copy link
Contributor

Yup.

bgilbert added a commit that referenced this issue Mar 17, 2022
golang.org/x/crypto/ssh finally supports RSA SHA-2 signatures, so we can
drop our workarounds for Fedora crypto policy forbidding RSA SHA-1.

Fixes #1772.  Reverts ac54a8a, db56cfe, and a2066db.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira for syncing to jira
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants