microsoft/azure: allow empty certificate chain in PKCS12 file #1074

marmijo · 2024-05-08T20:28:20Z

Azure is populating the certs endpoint even when an SSH key is not
provided. The certificate chain in the PKCS12 file is empty in this
case causing afterburn to fail with the current logic. Check if the
certificate chain is empty before retrieving the SSH public key
and handle this case as a valid option.

The more general crypto/mod.rs was updated here which also affected
the azurestack code. Update azurestack to accommodate the changes when
retrieving the SSH public key.

See: coreos/fedora-coreos-tracker#1553

dustymabe

mostly LGTM

Thanks for working on this!

src/providers/microsoft/azure/mod.rs

dustymabe · 2024-05-09T20:00:28Z

src/providers/microsoft/azure/mod.rs

+            Ok(k) => k,
+            Err(e) => match e.root_cause().to_string().as_ref() {
+                "SSH public key not found in chain" => {
+                    warn!("SSH pubkeys requested, but not provisioned for this instance");


I'm not sure this is the right warning to give to users, this makes it seem like we desired something (pubkeys requested), but couldn't find them.

maybe something like:

No SSH keys found in the certificate chain for this instance.

That sounds like a much better warning.
While we have this open, should I change the message a few lines up as well? I was attempting to show the warning we would have expected prior to coreos/fedora-coreos-tracker#1553

afterburn/src/providers/microsoft/azure/mod.rs

Line 406 in f1c93ed

warn!("SSH pubkeys requested, but not provisioned for this instance");

Since that warning would display when the certs endpoint doesn't exist, maybe that one could just read:

SSH pubkeys not provisioned for this instance

sounds good to me

src/providers/microsoft/crypto/mod.rs

dustymabe

LGTM

jlebon · 2024-05-10T19:15:47Z

docs/release-notes.md

@@ -12,6 +12,8 @@ Major changes:

 Minor changes:

+- Fix Azure SSH key fetching when no key provisioned.


Minor: we usually don't have a trailing period in these notes

jlebon · 2024-05-10T19:18:53Z

src/providers/microsoft/azure/mod.rs

+        let key = match self.get_ssh_pubkey(certs_endpoint) {
+            Ok(k) => k,
+            Err(e) => match e.root_cause().to_string().as_ref() {
+                "detected empty certificate chain" => {


So instead of string matching errors, a more rustic way to do this is to have get_ssh_pubkey() return a Result<Option<PublicKey>>. Then, get_ssh_pubkey() would return Ok(None) for this scenario. And here, we'd do e.g.:

let key: Vec<PublicKey> = self.get_ssh_pubkeys(certs_endpoint)?.unwrap_or_default();

I modified the line based on your suggestion. Since PublicKey doesn't implement Default, I used unwrap_or(vec![]) instead to assign an empty vector if a None value is returned.

jlebon · 2024-05-10T19:20:37Z

src/providers/microsoft/azure/mod.rs

+            Ok(k) => k,
+            Err(e) => match e.root_cause().to_string().as_ref() {
+                "detected empty certificate chain" => {
+                    warn!("No SSH keys found in the certificate chain for this instance");


Is this cause for warning?

maybe not warn, but at least an info log statement?

I'm not sure if it is cause for warning, but I chose to warn here because that's the behavior we expected when an SSH key wasn't supplied before this issue arose.

So to make sure I understand this: before, Azure would not offer up a certificate endpoint if no SSH keys were provided, but now it always does and instead signals no SSH keys through the certificate itself?

Basically, yes. I don't think Azure is directly signaling that no SSH key is provided through the certificate, but that's what we've found so far: that the certificate chain is empty when no SSH key is provided.

In that case, if the new behaviour is that the certificate chain is empty when no SSH keys are present, then it should not even be considered a warning since it's completely supported to not pass an SSH key. For the same reason as the other comment, I wouldn't even info! it (just like other providers don't info! if they didn't get a key), but not strongly against. We already log something when we do write SSH keys.

debug!() sounds appropriate as a debugging aid if we want and could live directly in src/providers/microsoft/crypto/mod.rs where we return Ok(None) so that the calling code here can stay simple with unwrap_or_default() (otherwise, to log it here, we'd need to inspect the Option first to know if it's None).

jlebon · 2024-05-10T19:23:35Z

src/providers/microsoft/azure/mod.rs

@@ -403,7 +403,7 @@ impl MetadataProvider for Azure {
        let certs_endpoint = match goalstate.certs_endpoint() {
            Some(ep) => ep,
            None => {
-                warn!("SSH pubkeys requested, but not provisioned for this instance");
+                warn!("SSH pubkeys not provisioned for this instance");


I think the "requested" wording in this warning refers to the fact that something called afterburn --ssh-keys USER but we couldn't find anything. And so at this layer of this stack, the warning as written previously was accurate. (Even though we know that at the FCOS layer, it's because we unconditionally run this service for the core user.)

what does afterburn --ssh-keys USER imply, though?

It's hard not to look at this from the lens of FCOS where we are saying essentially "if there are SSH keys give them to me".

After looking at the code for the other providers and the history of this line, I think we should drop this warning entirely. Other providers don't warn in the case that no SSH keys were found, and AFAICT there is no reason to treat Azure differently.

This test is still failing, but there's a PR open to resolve this. Let's snooze for another week so we can land: coreos/afterburn#1074

marmijo · 2024-06-19T23:53:33Z

I reworked this PR and updated the description based on the review comments.

This test is still failing. Let's snooze again while we wait for coreos/afterburn#1074 to land.

marmijo · 2024-06-21T19:29:22Z

The "Rust / Lints, pinned toolchain (pull_request)" check is failing on cargo fmt -- --check -l, but I get no errors when I run this command locally on this branch. What am I missing here?

This test is still failing. Let's snooze again while we wait for coreos/afterburn#1074 to land.

jlebon

The "Rust / Lints, pinned toolchain (pull_request)" check is failing on cargo fmt -- --check -l, but I get no errors when I run this command locally on this branch. What am I missing here?

This likely means that you're using a different version than what CI is using. You can see the version it's using in

afterburn/.github/workflows/rust.yml

Line 21 in 547c950

ACTIONS_LINTS_TOOLCHAIN: 1.75.0

and compare to yours. You don't have to match it; it should be enough to make sure you have the latest toolchain from Fedora (e.g. dnf update rust cargo clippy rustfmt) or from upstream if you used rustup (e.g. rustup update).

jlebon · 2024-06-21T20:29:19Z

src/providers/microsoft/azure/mod.rs


-        Ok(ssh_pubkey)
+        Ok(Some(vec![ssh_pubkey]))


How about making the signature of this function as Result<Option<PublicKey>> and then here it'd just be

Suggested change

Ok(Some(vec![ssh_pubkey]))

Ok(Some(ssh_pubkey))

Actually, I think once we do the above, we can shrink this whole thing to just call crypto::p12_to_ssh_pubkey(&p12).context("failed to convert pkcs12 blob to ssh pubkey") as the final line of the function since it already matches the signature we need.

jlebon · 2024-06-21T20:35:15Z

src/providers/microsoft/azure/mod.rs

-        let key = self.get_ssh_pubkey(certs_endpoint)?;
-        Ok(vec![key])
+        let key: Vec<PublicKey> = self.get_ssh_pubkey(certs_endpoint)?.unwrap_or(vec![]);
+
+        Ok(key)


Then this code here would remain untouched.

IOW, the lower-level get_ssh_pubkey() function can just focus on doing the obvious thing and this function here is where we have to deal with what our callers expect from us.

I was having a difficult time figuring out how to return a vector on None without inspecting the Option like

afterburn/src/providers/microsoft/azure/mod.rs

Lines 406 to 408 in c3aa867

let certs_endpoint = match goalstate.certs_endpoint() {

Some(ep) => ep,

None => return Ok(vec![]),

Oh right sorry, I missed that detail here. There's a neat Rust trick which is that Option actually supports iteration, in which case it'll iterate zero times or once (if the item exists). So something like Ok(maybe_key.into_iter().collect()) should work.

Perfect, thanks! I made that change.

jlebon

One nit, LGTM otherwise!

jlebon · 2024-06-21T21:56:14Z

src/providers/microsoft/azure/mod.rs

+        let ssh_pubkey = match crypto::p12_to_ssh_pubkey(&p12)
+            .context("failed to convert pkcs12 blob to ssh pubkey")?
+        {
+            Some(key) => key,
+            None => return Ok(None),
+        };


What I mean is that all this can now be

Suggested change

let ssh_pubkey = match crypto::p12_to_ssh_pubkey(&p12)

.context("failed to convert pkcs12 blob to ssh pubkey")?

{

Some(key) => key,

None => return Ok(None),

};

crypto::p12_to_ssh_pubkey(&p12)

.context("failed to convert pkcs12 blob to ssh pubkey")

And we can remove the Ok() line below.

Updated! Thank you.

I didnt update it properly. I'll work on it.

Azure is populating the certs endpoint even when an SSH key is not provided. The certificate chain in the PKCS12 file is empty in this case causing afterburn to fail with the current logic. Check if the certificate chain is empty before retrieving the SSH public key and handle this case as a valid option. The more general `crypto/mod.rs` was updated here which also affected the azurestack code. Update azurestack to accomodate the changes when retrieving the SSH public key.

jlebon

Nice work!

Extend the snooze on the `coreos.ignition.ssh.key` test failure for another month until a new release of Afterburn can be performed that will include the fix for the failure: coreos/afterburn#1074

marmijo force-pushed the marmijo-allow-empty-ca branch 2 times, most recently from 67b15ec to cbdb2a0 Compare May 8, 2024 20:40

marmijo mentioned this pull request May 8, 2024

coreos.ignition.ssh.key failing in kola-azure coreos/fedora-coreos-tracker#1553

Open

marmijo linked an issue May 8, 2024 that may be closed by this pull request

coreos.ignition.ssh.key failing in kola-azure coreos/fedora-coreos-tracker#1553

Open

dustymabe reviewed May 9, 2024

View reviewed changes

marmijo force-pushed the marmijo-allow-empty-ca branch 2 times, most recently from 2db1a5d to 250091c Compare May 9, 2024 21:42

dustymabe approved these changes May 10, 2024

View reviewed changes

jlebon reviewed May 10, 2024

View reviewed changes

marmijo mentioned this pull request May 13, 2024

denylist: extend snooze for coreos.ignition.ssh.key coreos/fedora-coreos-config#2999

Merged

marmijo force-pushed the marmijo-allow-empty-ca branch 3 times, most recently from 28baf16 to 388b98d Compare June 19, 2024 23:51

marmijo added a commit to marmijo/fedora-coreos-config that referenced this pull request Jun 21, 2024

denylist: extend snooze on coreos.ignition.ssh.key

d2b3dab

This test is still failing. Let's snooze again while we wait for coreos/afterburn#1074 to land.

marmijo mentioned this pull request Jun 21, 2024

denylist: extend snooze on coreos.ignition.ssh.key coreos/fedora-coreos-config#3035

Merged

marmijo force-pushed the marmijo-allow-empty-ca branch 2 times, most recently from 458dafc to c3aa867 Compare June 21, 2024 19:24

jlebon pushed a commit to coreos/fedora-coreos-config that referenced this pull request Jun 21, 2024

denylist: extend snooze on coreos.ignition.ssh.key

95659b1

This test is still failing. Let's snooze again while we wait for coreos/afterburn#1074 to land.

jlebon reviewed Jun 21, 2024

View reviewed changes

marmijo force-pushed the marmijo-allow-empty-ca branch 2 times, most recently from a032055 to 21b1e89 Compare June 21, 2024 21:34

jlebon approved these changes Jun 21, 2024

View reviewed changes

marmijo force-pushed the marmijo-allow-empty-ca branch 2 times, most recently from a82baf2 to 04be6a4 Compare June 21, 2024 22:04

marmijo requested a review from jlebon June 21, 2024 22:05

marmijo force-pushed the marmijo-allow-empty-ca branch from 04be6a4 to 622b0d0 Compare June 21, 2024 22:07

jlebon approved these changes Jun 21, 2024

View reviewed changes

jlebon enabled auto-merge June 21, 2024 22:40

jlebon merged commit 4dfc444 into coreos:main Jun 21, 2024
9 checks passed

marmijo mentioned this pull request Jul 1, 2024

denylist: snooze ssh.key until new afterburn release coreos/fedora-coreos-config#3046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

microsoft/azure: allow empty certificate chain in PKCS12 file #1074

microsoft/azure: allow empty certificate chain in PKCS12 file #1074

marmijo commented May 8, 2024 •

edited

Loading

dustymabe left a comment

dustymabe May 9, 2024

dustymabe May 9, 2024

marmijo May 9, 2024

marmijo May 9, 2024

marmijo May 9, 2024

dustymabe May 9, 2024

dustymabe left a comment

jlebon May 10, 2024

jlebon May 10, 2024

marmijo Jun 19, 2024

jlebon May 10, 2024

dustymabe May 10, 2024

marmijo May 10, 2024

jlebon May 10, 2024

marmijo May 10, 2024

jlebon May 10, 2024

jlebon May 10, 2024

dustymabe May 10, 2024

jlebon May 10, 2024

marmijo commented Jun 19, 2024

marmijo commented Jun 21, 2024

jlebon left a comment

jlebon Jun 21, 2024

jlebon Jun 21, 2024

jlebon Jun 21, 2024

marmijo Jun 21, 2024

jlebon Jun 21, 2024 •

edited

Loading

marmijo Jun 21, 2024

jlebon left a comment

jlebon Jun 21, 2024

marmijo Jun 21, 2024

marmijo Jun 21, 2024

jlebon left a comment

		@@ -12,6 +12,8 @@ Major changes:

		Minor changes:

		- Fix Azure SSH key fetching when no key provisioned.

	let certs_endpoint = match goalstate.certs_endpoint() {
	Some(ep) => ep,
	None => return Ok(vec![]),

microsoft/azure: allow empty certificate chain in PKCS12 file #1074

microsoft/azure: allow empty certificate chain in PKCS12 file #1074

Conversation

marmijo commented May 8, 2024 • edited Loading

dustymabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmijo commented Jun 19, 2024

marmijo commented Jun 21, 2024

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

marmijo commented May 8, 2024 •

edited

Loading

jlebon Jun 21, 2024 •

edited

Loading