feat: manage ssh certificates #252

EmyLIEUTAUD · 2023-08-31T12:27:18Z

Enhancement:

Deploy User CA on the system
Configure principals (optional)

Reason:
This allows you to configure and manage the SSH server to authenticate via certificates.
Improves SSH authentication security: certificates have a validity period, unlike SSH keys.

More information on SSH certificates is available here: Managing SSH Access at Scale with HashiCorp Vault.

Result:
All tests passed.
The related documentation is available and an example can be found in examples/example-use-certificates.yml.

Issue Tracker Tickets (Jira or BZ if any): -

…th principals

Certificates

Jakuje · 2023-08-31T12:55:01Z

tasks/certificates.yml

+- name: Copy Trusted user CA Keys
+  ansible.builtin.template:
+    src: "trusted-user-ca-keys.pem.j2"
+    dest: "{{ sshd['TrustedUserCAKeys'] }}"
+    mode: '0600'


I find it confusing that you are making a file with .pem extension which lists SSH keys in non-PEM format.

I used the extension given as an example in the HashiCorp tutorial. In any case, the extension doesn't matter. Do you know a format that would be suitable without being confusing?

Generally the SSH public keys have the .pub extension so I would probably go with that one.

tests/tests_set_common.yml

Jakuje · 2023-08-31T12:57:26Z

defaults/main.yml

+# If not null, list of trusted CA keys
+sshd_trusted_user_ca_keys_list: []


comment does not math the default value here. Probably mention empty instead of null in the comment?

templates/auth_principals.j2

templates/trusted-user-ca-keys.pub.j2

mattwillsher

One spelling correction but otherwise lgtm

tasks/certificates.yml

examples/example-use-certificates.yml

tasks/certificates.yml

spetrosi · 2023-09-06T11:44:20Z

tasks/certificates.yml

+    src: "auth_principals.j2"
+    dest: "{{ sshd['AuthorizedPrincipalsFile'] | dirname }}/{{ item.key }}"
+    mode: '0644'
+  loop: "{{ q('dict', sshd_principals) }}"


Suggested change

loop: "{{ q('dict', sshd_principals) }}"

loop: "{{ ('dict', sshd_principals) }}"

Is it a typo?

This was an old syntax we used, but it's no longer documented. I've modified it to make the code easier to understand.

spetrosi · 2023-09-06T11:44:42Z

tests/tests_set_common.yml

+
+        - name: Check AuthorizedPrincipalsFile exists
+          ansible.builtin.assert:
+            that:


Suggested change

that:

that: authorizedprincipalsfile_stat.stat.exists

spetrosi · 2023-09-06T11:44:50Z

tests/tests_set_common.yml

+        - name: Check AuthorizedPrincipalsFile exists
+          ansible.builtin.assert:
+            that:
+              - authorizedprincipalsfile_stat.stat.exists


Suggested change

- authorizedprincipalsfile_stat.stat.exists

spetrosi · 2023-09-06T11:45:21Z

tests/tests_set_common.yml

+
+        - name: Check TrustedUserCAKeys file exists
+          ansible.builtin.assert:
+            that:


Suggested change

that:

that: trustedusercakeys_file_stat.stat.exists

spetrosi · 2023-09-06T11:45:30Z

tests/tests_set_common.yml

+        - name: Check TrustedUserCAKeys file exists
+          ansible.builtin.assert:
+            that:
+              - trustedusercakeys_file_stat.stat.exists


Suggested change

- trustedusercakeys_file_stat.stat.exists

spetrosi · 2023-09-06T14:10:36Z

lgtm

tasks/certificates.yml

Jakuje

hope last nits to the wording of README. Then I would love to have the fixup commits slightly squashed as the commit series is getting long ...

README.md

build(deps): bump actions/checkout from 3 to 4 Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> spelling correction use random ssh key fix ansible lint fix test syntax update dictionary loop syntax update README and add variables to configure directories and files

Jakuje · 2023-09-07T14:22:19Z

.pre-commit-config.yaml

@@ -8,7 +8,7 @@ repos:
        types: [file, yaml]
        entry: yamllint --strict
  - repo: https://github.com/ansible/ansible-lint.git
-    rev: v6.5.2
+    rev: v6.17.2


please, keep the dependencies changes in at least separate commit (ideally separate PR).

Version restored here: 610a356.

README.md

tests/tests_set_common.yml

add teststo certificates and restore ansible-lint version fix tests for certificates build(deps): bump actions/checkout from 3 to 4 Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>

mattwillsher

Minor ordering in docs but looks good to me otherwise.

I don't understand why the checkout action is showing the version change - have you rebased from main recently? It might resolve that

README.md

fix README change trusted user ca keys directory default mode

Jakuje

code wise it looks good. I would prefer the fixups commits squashed, but if others are ok with the commit series. I do not want to block the changes

mattwillsher · 2023-09-11T13:40:09Z

Thanks for all the work on this @EmyLIEUTAUD

I'll get a new release out that includes this code in the few days.

ubuntu and others added 11 commits August 9, 2023 17:44

Role configured to accept SSH connection via SSH certificates

e24942c

Works with or without principals and ansible-lint updated

7d7ca39

Change sshd_principals description

346b720

add test for SSH certificates authentication with principals

b3d9685

Add configuration to run tests for SSH certificates authentication wi…

d95f2c6

…th principals

Merge branch 'willshersystems:master' into master

2a4307f

tasks to use SSH certificates grouped into one file

e58ea0b

Update README.md

0f272ac

Code enhancement

1196d11

Merge branch 'master' into certificates

63aa9a3

Merge pull request #2 from EmyLIEUTAUD/certificates

9f5a9e5

Certificates

EmyLIEUTAUD mentioned this pull request Aug 31, 2023

Proposition to manage SSH Certificates #177

Open

mattwillsher changed the title ~~Manage SSH Certificates~~ feat: Manage SSH Certificates Aug 31, 2023

Jakuje mentioned this pull request Aug 31, 2023

docs(changelog): version v0.21.0 #251

Closed

Jakuje reviewed Aug 31, 2023

View reviewed changes

EmyLIEUTAUD added 3 commits August 31, 2023 15:34

improved certificate testing

44d8939

Fix markdown lint test

453957b

use .pub instead of .pem for TrustedUserCAKeys file

d8c530b

spetrosi reviewed Sep 4, 2023

View reviewed changes

templates/auth_principals.j2 Show resolved Hide resolved

templates/trusted-user-ca-keys.pub.j2 Show resolved Hide resolved

EmyLIEUTAUD added 2 commits September 4, 2023 13:35

add Ansible managed comment to jinja2 templates

fc57b6e

add willshersystems:ansible-sshd comment to jinja2 templates

c600936

EmyLIEUTAUD changed the title ~~feat: Manage SSH Certificates~~ feat: manage ssh certificates Sep 6, 2023

mattwillsher requested changes Sep 6, 2023

View reviewed changes

tasks/certificates.yml Outdated Show resolved Hide resolved

Jakuje requested changes Sep 6, 2023

View reviewed changes

examples/example-use-certificates.yml Outdated Show resolved Hide resolved

tasks/certificates.yml Outdated Show resolved Hide resolved

spetrosi reviewed Sep 6, 2023

View reviewed changes

richm reviewed Sep 6, 2023

View reviewed changes

tasks/certificates.yml Outdated Show resolved Hide resolved

Jakuje reviewed Sep 6, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

EmyLIEUTAUD force-pushed the master branch from 688cbc8 to dfd2e93 Compare September 7, 2023 14:08

Jakuje reviewed Sep 7, 2023

View reviewed changes

tests/tests_set_common.yml Outdated Show resolved Hide resolved

EmyLIEUTAUD force-pushed the master branch from 2cc041a to 610a356 Compare September 8, 2023 09:09

EmyLIEUTAUD requested review from Jakuje and mattwillsher September 8, 2023 12:38

mattwillsher requested changes Sep 8, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

mattwillsher approved these changes Sep 8, 2023

View reviewed changes

richm approved these changes Sep 8, 2023

View reviewed changes

Check README and default mode for directories

86b3c81

fix README change trusted user ca keys directory default mode

EmyLIEUTAUD force-pushed the master branch from 70c0e36 to 86b3c81 Compare September 8, 2023 14:38

Jakuje approved these changes Sep 8, 2023

View reviewed changes

mattwillsher merged commit 0bc6d8f into willshersystems:main Sep 11, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: manage ssh certificates #252

feat: manage ssh certificates #252

EmyLIEUTAUD commented Aug 31, 2023

Jakuje Aug 31, 2023

EmyLIEUTAUD Sep 1, 2023

Jakuje Sep 1, 2023

Jakuje Aug 31, 2023

mattwillsher left a comment •

edited

Loading

spetrosi Sep 6, 2023

EmyLIEUTAUD Sep 6, 2023

spetrosi Sep 6, 2023

spetrosi Sep 6, 2023

spetrosi Sep 6, 2023

spetrosi Sep 6, 2023

spetrosi commented Sep 6, 2023

Jakuje left a comment

Jakuje Sep 7, 2023

EmyLIEUTAUD Sep 8, 2023

mattwillsher left a comment •

edited

Loading

Jakuje left a comment

mattwillsher commented Sep 11, 2023

		# If not null, list of trusted CA keys
		sshd_trusted_user_ca_keys_list: []

	loop: "{{ q('dict', sshd_principals) }}"
	loop: "{{ ('dict', sshd_principals) }}"

feat: manage ssh certificates #252

feat: manage ssh certificates #252

Conversation

EmyLIEUTAUD commented Aug 31, 2023

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

mattwillsher left a comment • edited Loading

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

spetrosi commented Sep 6, 2023

Jakuje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwillsher left a comment • edited Loading

Choose a reason for hiding this comment

Jakuje left a comment

Choose a reason for hiding this comment

mattwillsher commented Sep 11, 2023

mattwillsher left a comment •

edited

Loading

mattwillsher left a comment •

edited

Loading