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

Add support for CA bundles for fetching the ignition config #968

Merged
merged 4 commits into from
May 15, 2020

Conversation

sohankunkerkar
Copy link
Member

Fixes #931

@coreosbot
Copy link
Contributor

Can one of the admins verify this patch?

@LorbusChris
Copy link
Contributor

ok to test

@LorbusChris
Copy link
Contributor

LorbusChris commented May 1, 2020

god the mobile UI has me confused. What is needed here to make coreosbot happ? 😅

@sohankunkerkar sohankunkerkar force-pushed the addCabundle branch 6 times, most recently from 4b84ef9 to ae6dde7 Compare May 1, 2020 15:10
@sohankunkerkar sohankunkerkar changed the title WIP: Add support for a CA bundle for fetching the ignition config WIP: Add support for CA bundles for fetching the ignition config May 1, 2020
@sohankunkerkar sohankunkerkar force-pushed the addCabundle branch 12 times, most recently from dde3cc7 to 13f229a Compare May 6, 2020 12:01
@jlebon
Copy link
Member

jlebon commented May 6, 2020

ok to test

@jlebon
Copy link
Member

jlebon commented May 6, 2020

Can you try rebasing on the latest master?

@sohankunkerkar sohankunkerkar force-pushed the addCabundle branch 2 times, most recently from 916f609 to d103567 Compare May 7, 2020 23:55
@sohankunkerkar sohankunkerkar marked this pull request as ready for review May 8, 2020 01:11
@sohankunkerkar sohankunkerkar force-pushed the addCabundle branch 2 times, most recently from 64bc230 to 2d47757 Compare May 13, 2020 13:33
@sohankunkerkar sohankunkerkar force-pushed the addCabundle branch 2 times, most recently from 88b6b44 to 7d24484 Compare May 13, 2020 14:39
@bgilbert
Copy link
Contributor

As currently implemented, this change is retroactive to all spec versions. That creates a problem: if someone writes a 3.0 config that uses this functionality, and passes it to an older version of Ignition, the second and subsequent certs will be ignored. (Which, if it affected the outcome at all, would cause fetch failures elsewhere in the config -- so the issue would be detected at least.)

The other approach is to make this change only in the -experimental spec, and on older config versions, intentionally parse only the first cert. That's more explicit, but would substantially delay rollout of the functionality, and requires us to add code to intentionally limit certificate parsing. @miabbott @cgwalters @lucab @jlebon thoughts?

If we continue on the current path, the config spec docs (all versions) should be updated to specify that a source can contain multiple CA certs.

Once complete, this PR should probably also be backported to spec2x.

tests/fixtures/tls_fixtures.go Outdated Show resolved Hide resolved
tests/fixtures/tls_fixtures.go Show resolved Hide resolved
tests/fixtures/tls_fixtures.go Show resolved Hide resolved
tests/negative/security/tls.go Outdated Show resolved Hide resolved
tests/negative/security/tls.go Outdated Show resolved Hide resolved
tests/positive/security/tls.go Outdated Show resolved Hide resolved
tests/positive/security/tls.go Outdated Show resolved Hide resolved
tests/servers/servers.go Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar force-pushed the addCabundle branch 4 times, most recently from acf66b0 to ddfd7cc Compare May 14, 2020 14:52
@jlebon
Copy link
Member

jlebon commented May 14, 2020

The other approach is to make this change only in the -experimental spec, and on older config versions, intentionally parse only the first cert. That's more explicit, but would substantially delay rollout of the functionality, and requires us to add code to intentionally limit certificate parsing. @miabbott @cgwalters @lucab @jlebon thoughts?

Hmm, it's debatable, though I think I see this more as a bugfix than a spec change. IOW, one could see the previous behaviour as wrong and this PR fixing it for all spec versions. Or a related question: could parsing more certificates now reasonably cause a regression? If not, then I think it's fine to just let it get into all the specs even though we're technically changing the semantics of that field.

@arithx
Copy link
Contributor

arithx commented May 14, 2020

I think I'd agree with @jlebon that we can just view this as a bugfix and push to all versions.

@bgilbert
Copy link
Contributor

I'm okay with making the change for all spec versions, as we're doing now. @sohankunkerkar, please clarify the wording in the spec docs as well.

@sohankunkerkar sohankunkerkar force-pushed the addCabundle branch 2 times, most recently from 41d1ae6 to de46364 Compare May 15, 2020 00:40
@lucab
Copy link
Contributor

lucab commented May 15, 2020

I imagine that a likely outcome of this is that it will start detecting malformed bundles with trailing garbage. Even if that may break some previously-accepted configs, I tend to agree with @jlebon that this still fits into the "soundness bugfix" bucket.

@miabbott
Copy link
Member

FWIW, I'm in agreement with the rest...I see this as more of a bug fix than anything else.

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.

Support CA bundles for Ignition config
9 participants