New feature: ca_certs support RHEL by cawamata · Pull Request #633 · canonical/cloud-init

cawamata · 2020-10-28T15:50:40Z

Proposed Commit Message

New feature of ca_certs, support RHEL

Test Steps

user-data: same as Ubuntu case
on RHEL Server, I confirmed the followings

/usr/share/pki/ca-trust-source/anchors/cloud-init-ca-certs.crt exist
new certificate is included by trust list

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

igalic · 2020-10-28T17:11:17Z

this is really good timing, becausei just opened https://bugs.launchpad.net/cloud-init/+bug/1901915 to add FreeBSD support, so this is fantastic groundwork!

github-actions · 2020-11-18T00:20:05Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging mitechie, and he will ensure that someone takes a look soon.

(If the pull request is closed, please do feel free to reopen it if you wish to continue working on it.)

igalic · 2020-11-18T05:44:03Z

@mitechie yes, this is awaiting code review and it already has someone assigned

OddBloke

Hi @cawamata, thanks for taking the time to work on this and improve cloud-init! I have some comment on the code inline. Let me know if you have any questions!

I'd also like to see the testing here expanded: currently it only covers Ubuntu's behaviour in most cases, and we should at least be covering Red Hat's too. Is expanding the testing something you feel comfortable doing?

Thanks again!

OddBloke · 2020-11-19T20:45:41Z

-CA_CERT_FULL_PATH = os.path.join(CA_CERT_PATH, CA_CERT_FILENAME)
-
-distros = ['alpine', 'debian', 'ubuntu']
+DISTRO_DEFAULT_CONFIG = {


There's a lot of repetition in here; perhaps it's worth having a DEFAULT_CONFIG and a DISTRO_OVERRIDES, so we only capture the Ubuntu/Debian/Alpine settings once? _distro_ca_certs_config would then look in DISTRO_OVERRIDES and return a value found in there, otherwise return DEFAULT_CONFIG.

What do you think?

I'm not sure Ubuntu/Debian/Alpine is set as DEFAULT_CONFIG, because I don't know cloud-init's default distros..
However, if we can use I agree your proposal

cloud-init doesn't really have default distros. All I mean here is that those three distros share a common config, so we only need to store it once; as that'll be what we use if there isn't a more specific configuration to use, it is the default configuration, by definition.

Does that make sense?

OddBloke · 2020-11-19T21:25:33Z

+    """Construct a distro-specific ca_certs config dictionary by merging
+       distro specific changes into base config.


This docstring doesn't sound quite right: this doesn't perform any merging, and we don't have a base config.

OddBloke · 2020-11-19T21:25:43Z

+       distro specific changes into base config.
+
+    @param distro: String providing the distro class name.
+    @returns: Dict of distro configurations for ntp clients.


OddBloke · 2020-11-19T21:28:43Z

    @param certs: A list of certificate strings.
    """
+    distro_cfg = _distro_ca_certs_configs(distro_name)
    if certs:


You didn't add this line, but as you're already modifying this entire function, inverting this to:

if not certs: return

would allow you to dedent everything else in here by one step.

(that's one of my favourite things to do)

OddBloke · 2020-11-19T21:29:37Z

+    'rhel': {
+        'ca_cert_path': '/usr/share/pki/ca-trust-source/',
+        'ca_cert_filename': 'anchors/cloud-init-ca-certs.crt',
+        'ca_cert_config': '',


It's preferable to use None to indicate absence of a value, rather than the empty value.

OddBloke · 2020-11-19T21:31:23Z

+            if os.stat(distro_cfg['ca_cert_config']).st_size == 0:
+                # If the CA_CERT_CONFIG file is empty (i.e. all existing
+                # CA certs have been deleted) then simply output a single
+                # line with the cloud-init cert filename.
+                out = "%s\n" % distro_cfg['ca_cert_filename']
+            else:
+                # Append cert filename to CA_CERT_CONFIG file.
+                # We have to strip the content because blank lines in the file
+                # causes subsequent entries to be ignored. (LP: #1077020)
+                orig = util.load_file(distro_cfg['ca_cert_config'])
+                cr_cont = '\n'.join([line for line in orig.splitlines()
+                                    if line != distro_cfg['ca_cert_filename']])
+                out = "%s\n%s\n" % (cr_cont.rstrip(),
+                                    distro_cfg['ca_cert_filename'])
+            util.write_file(distro_cfg['ca_cert_config'], out, omode="wb")


This feels like it could quite naturally be refactored to a separate function; what do you think?

current function is included add_ca_certs and update_cert_config, so I agree to separate this

OddBloke · 2020-11-19T21:32:41Z

+    util.write_file(distro_cfg['ca_cert_config'], "", mode=0o644)

-    if distro_name != 'alpine':
+    if distro_name == 'debian' or distro_name == 'ubuntu':


Suggested change

if distro_name == 'debian' or distro_name == 'ubuntu':

if distro_name in ['debian', 'ubuntu']:

reflect PR review comments

igalic

🙋🏻‍♀️

Co-authored-by: Mina Galić <me+github@igalic.co>

igalic

i'd like to see more explicit rhel tests

igalic · 2020-11-25T22:32:45Z

+    'ca_cert_filename': 'cloud-init-ca-certs.crt',
+    'ca_cert_config': '/etc/ca-certificates.conf',
+    'ca_cert_system_path': '/etc/ssl/certs/',
+    'ca_cert_update_exe': 'update-ca-certificates'


i would call this ca_cert_update_cmd and make it an array, which is what subp expects

I updated, please check

igalic · 2020-11-25T22:33:46Z

    """
-    subp.subp(["update-ca-certificates"], capture=False)
+    distro_cfg = _distro_ca_certs_configs(distro_name)
+    subp.subp([distro_cfg['ca_cert_update_exe']], capture=False)


that way, we're ready for a future distro where the update command is actually a sub-command, that this function cannot possibly know about

igalic

this looks pretty good from my perspective

cawamata · 2020-12-16T07:22:16Z

@OddBloke san
Sorry for late, I updated test case.
Can you re-review this?

OddBloke

Thanks for the update @cawamata! I have three inline comments: the most important is the third, where I propose a modification to pass the config dict around. Let me know what you think!

OddBloke · 2020-12-16T17:36:32Z


-def update_ca_certs():
+
+def _distro_ca_certs_configs(distro):


A nit (so no need to address it for this to land), but distro is used elsewhere in this file to mean "a Distro instance"; using distro_name instead would avoid that.

I updated, please check

OddBloke · 2020-12-16T17:46:00Z

+    @param distro: String providing the distro class name.
    """
-    subp.subp(["update-ca-certificates"], capture=False)
+    distro_cfg = _distro_ca_certs_configs(distro_name)


We call _distro_ca_certs_config(distro_name) in many different places in this PR. Do you think it would make sense to instead call it once (in handle) and then pass the configuration dict itself around?

Okay, to update this, I have to update test case, too.
So I will try

I updated, please confirm

OddBloke

Thanks for all the work here, @cawamata, this LGTM now!

cawamata added 3 commits October 29, 2020 00:23

add rhel support

b294f36

add rhel test case

dcd2165

Update cc_ca_certs.py

bb55661

cawamata force-pushed the ca_certs_add_rhel branch 2 times, most recently from 59fff6f to bb55661 Compare October 28, 2020 23:23

cawamata added 2 commits October 29, 2020 08:48

add cawamata

8ac34e6

modify from flake8 test result

2984764

OddBloke self-assigned this Nov 3, 2020

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Nov 18, 2020

OddBloke removed the stale-pr Pull request is stale; will be auto-closed soon label Nov 18, 2020

OddBloke suggested changes Nov 19, 2020

View reviewed changes

Update cc_ca_certs.py

4041e8f

reflect PR review comments

igalic suggested changes Nov 25, 2020

View reviewed changes

Comment thread cloudinit/config/cc_ca_certs.py Outdated

Update cloudinit/config/cc_ca_certs.py

f11b783

Co-authored-by: Mina Galić <me+github@igalic.co>

igalic suggested changes Nov 25, 2020

View reviewed changes

cawamata added 4 commits December 7, 2020 22:44

Update cc_ca_certs.py

aa42fe0

Update test_handler_ca_certs.py

c9ca592

Update test_handler_ca_certs.py

d4ed29d

Update cc_ca_certs.py

f894334

igalic approved these changes Dec 7, 2020

View reviewed changes

cawamata added 3 commits December 16, 2020 15:14

Update test_handler_ca_certs.py

dd13794

Merge branch 'master' into ca_certs_add_rhel

7849ea8

Update test_handler_ca_certs.py

76faf46

OddBloke reviewed Dec 16, 2020

View reviewed changes

cawamata added 2 commits December 17, 2020 12:26

Update cc_ca_certs.py

a683666

Update cc_ca_certs.py

c137f00

cawamata and others added 2 commits December 17, 2020 13:04

Update test_handler_ca_certs.py

082a67a

Merge branch 'master' into ca_certs_add_rhel

be8b739

OddBloke approved these changes Dec 17, 2020

View reviewed changes

OddBloke merged commit a5484d0 into canonical:master Dec 17, 2020

This was referenced May 12, 2023

Release 21.1 #3846

Closed

Randomly set credentials written in cleartext to world-readable file #3854

Closed

		"""Construct a distro-specific ca_certs config dictionary by merging
		distro specific changes into base config.

	if distro_name == 'debian' or distro_name == 'ubuntu':
	if distro_name in ['debian', 'ubuntu']:


		def update_ca_certs():

		def _distro_ca_certs_configs(distro):

Conversation

cawamata commented Oct 28, 2020

Proposed Commit Message

Test Steps

Checklist:

Uh oh!

igalic commented Oct 28, 2020

Uh oh!

github-actions Bot commented Nov 18, 2020

Uh oh!

igalic commented Nov 18, 2020

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

cawamata commented Dec 16, 2020

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!