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

Provide CA Certificates via cloud-init's ca_certs module #262

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

AlexVulaj
Copy link
Contributor

@AlexVulaj AlexVulaj commented Aug 5, 2024

  • Bug

What does this PR do? / Related Issues / Jira

Currently, the curl flag we're using to provide a ca cert will only use the provided cert - we want to use both the supplied SSL cert and RHEL's OEM trust bundle. This change moves away from the curl flag and instead uses the ca_certs module of cloud-init.

Resolves OSD-24353

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have tested the functionality against gcp / aws, it doesn't cause any regression
  • I have added execution results to the PR's readme

How to test this PR locally / Special Instructions

See OSD-24353 for setting up a transparent proxy. I used https://gitlab.cee.redhat.com/travi/osd-proxy-infra for setting up a non-transparent proxy.

I ran these new changes against

  • a transparent proxy
  • a non-transparent proxy
  • a split-cert transparent proxy

In all situations, I saw the expected results. Elaborating more on the split-cert situation, I set up a transparent proxy using mitmproxy with --ignore-hosts registry.redhat.io:443 in the service file. When I ran network verifier without passing a capath, all URLs failed EXCEPT registry.redhat.io

Logs

With this change, the userdata template expands to something like the following:

#cloud-config

ca_certs:
  trusted:
    - |-
      -----BEGIN CERTIFICATE-----
      THE CERTIFICATE
      -----END CERTIFICATE-----
runcmd:
  - systemctl mask --now serial-getty@ttyS0.service
  - dmesg -D
  - echo "NV_CURLJSON_BEGIN" >/dev/ttyS0
  - export http_proxy= https_proxy=
  - curl --capath /etc/pki/tls/certs/ --retry 3 --retry-connrefused -t B -Z -s -I -m ${TIMEOUT} -w "%{stderr}${LINE_PREFIX}%{json}\n" ${CURLOPT} ${URLS} --proto =http,https,telnet ${TLSDISABLED_URLS_RENDERED} 2>/dev/ttyS0
  - echo "NV_CURLJSON_END" >/dev/ttyS0
power_state:
  delay: 2
  mode: poweroff
  message: Auto-terminating instance due to timeout
  timeout: 300

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 25.27%. Comparing base (87b6710) to head (9152c0f).
Report is 2 commits behind head on main.

Files Patch % Lines
pkg/probes/curl/curl_json.go 88.88% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
+ Coverage   24.82%   25.27%   +0.44%     
==========================================
  Files          23       24       +1     
  Lines        1740     1812      +72     
==========================================
+ Hits          432      458      +26     
- Misses       1286     1329      +43     
- Partials       22       25       +3     
Files Coverage Δ
pkg/probes/curl/curl_json.go 76.59% <88.88%> (-1.46%) ⬇️

... and 3 files with indirect coverage changes

pkg/verifier/aws/entry_point.go Outdated Show resolved Hide resolved
Comment on lines 111 to 138
if cacert := userDataVariables["CACERT"]; cacert != "" {
type CaCert struct {
Trusted []string `yaml:"trusted"`
}
type CloudConfig struct {
CaCerts CaCert `yaml:"ca_certs"`
}

cloudInit := CloudConfig{
CaCerts: CaCert{
Trusted: []string{
strings.TrimSpace(cacert),
},
},
}

cloudInitYamlBytes, cloudInitMarshalErr := yaml.Marshal(&cloudInit)
if cloudInitMarshalErr != nil {
return "", fmt.Errorf("unable to create cloud init config: %w", cloudInitMarshalErr)
}

userDataVariables["CACERT_RENDERED"] = strings.TrimSpace(string(cloudInitYamlBytes))
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat approach, and smart way of avoiding complicated YAML escaping

@AlexVulaj AlexVulaj force-pushed the ca_certs_cloud_init branch 6 times, most recently from b9ba37e to 2ff842e Compare August 9, 2024 16:04
Copy link
Contributor

@abyrne55 abyrne55 left a comment

Choose a reason for hiding this comment

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

Very good stuff. I re-ran tests behind a transparent proxy to great success. I was then able to tweak my terraform testing scripts enough to get a non-transparent proxy working, and that's when I realized that curl does some extra SSL certificate checks when non-transparent proxies are specified (e.g., via osd-network-verifier egress --https-proxy="https://...). So the suggestion below just tells curl to check the same trust store dir for proxy SSL certs as it does for "unproxied" (or transparently proxied) SSL certs.

pkg/probes/curl/userdata-template.yaml Outdated Show resolved Hide resolved
Co-authored-by: Anthony Byrne <abyrne@redhat.com>
@AlexVulaj AlexVulaj force-pushed the ca_certs_cloud_init branch from 2e680a3 to 9152c0f Compare August 13, 2024 13:09
@abyrne55
Copy link
Contributor

Tested locally. Nice work!

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2024
Copy link
Contributor

openshift-ci bot commented Aug 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abyrne55

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 Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 42ab248 into openshift:main Aug 13, 2024
6 checks passed
@AlexVulaj AlexVulaj deleted the ca_certs_cloud_init branch August 13, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants