T2405: add Git support to commit-archive #2241

yunzheng · 2023-09-11T15:51:35Z

Change Summary

This change adds Git support to commit-archive. Example commit-archive locations for Git:

git://git@github.com:username/vyos-configs.git
git+ssh://git@github.com/username/vyos-configs.git
git+https://gitea.local.lan/infra/vyos-configs.git

The commited filename will be config.boot-<hostname>. So make sure you have unique hosts names if you let multiple VyOS servers commit to the same repository.

The username and Full Name will be extracted from the system and used as the author of the Git commit.
The "commit comment" is also properly used as the Git commit message:

vyos@vyos# commit comment "my commit comment"

It's recommended to use dedicated SSH keys instead of username and passwords. Therefore, the Git commit-archive backend checks if the following SSH private key exists and uses it accordingly:

/config/auth/commit-archive.key

You can generate the SSH key using:

$ generate pki ssh-key file /config/auth/commit-archive
Enter private key type: [rsa, dsa, ec] (Default: rsa)
Enter private key bits: (Default: 2048)
Note: If you plan to use the generated key on this router, do not encrypt the private key.
Do you want to encrypt the private key with a passphrase? [y/N]
File written to /config/auth/commit-archive.pem
File written to /config/auth/commit-archive.key

Then use the public key as a "deploy key" with write access on the Git repository.

PS: it would also be nice to generate ed25519 keys from VyOS, but that's another feature.
If you really want to use ed25519 keys you can just generate it using:

ssh-keygen -t ed25519 -f /config/auth/commit-archive.key

Just make sure that the private key is readable by the vyattacfg group.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

I refactored the upload and download function to have an extra parameter called raise_error=False.
The default is False, but when set to true it will raise the Exception instead of printing the error.

Several functions in process.py are also slightly refactored so they can properly accept a list of arguments, instead of assuming it is always a single str argument.

Related Task(s)

Component(s) name

config-management

Proposed changes

The change adds the following extra debian package:

git

Several changes needed to be made, I opted in to use a "regex" for the location validation instead of relying on the validate-value --file-transports which would require a recompile of vyos-utils package. It was also missing "ssh://" as a valid scheme, while this is listed as a working backend.

Changes have been made to remote.py to add GitC as a backend, I did not add support for download but could be done if there is a need for it.

Small textual changes have been made to config_mgmt.py to mimic the old VyOS commit-archive output. e.g:

vyos@vyos# commit
Archiving config...
  git+ssh://git@gitea.local.lan:2222/infra/vyos-configs.git OK
  git://git@github.com:username/vyos-configs.git OK

Before, nothing was outputted before.

Note that SSH Authorized keys is a thing, especially if there are multiple administrators this can be a hassle that everyone needs to accept the SSH key without knowing if it's correct. Therefore it's recommended to directly configure the correct authorized_keys file globally in /etc/ssh/ssh_known_hosts. For example GitHub lists their public keys here:

https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/githubs-ssh-key-fingerprints

But otherwise you can also utilize: ssh-keyscan, eg:

ssh-keyscan github.com | sudo tee -a /etc/ssh/ssh_known_hosts
ssh-keyscan -p 2222 gitea.local.lan | sudo tee -a /etc/ssh/ssh_known_hosts

How to test

First generate a dedicated SSH key for the commit-archive that can be used as a "deploy key", for example on GitHub, Gitea or GitLab.

vyos@vyos:~$ generate pki ssh-key file /config/auth/commit-archive
(you can accept all default answers)
vyos@vyos:~$ cat /config/auth/commit-archive.key.pub

Configure the above public key as a new deploy key on your Git repository with write access. Then configure the commit-archive location:

vyos@vyos:~$ config
vyos@vyos# set system config-management commit-archive location git://git@github.com:username/vyos-configs.git
vyos@vyos# commit
Archiving config...
  git://git@github.com:username/vyos-configs.git OK
[edit]
vyos@vyos#

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the components SMOKETESTS if applicable
My commit headlines contain a valid Task id
My change requires a change to the documentation
I have updated the documentation accordingly

yunzheng · 2023-09-11T16:02:08Z

This check fails: "Check pull request message format / Check pull request title (pull_request)"
i'm not sure how to fix the pull request message without redoing all my commits. Will the commits not be squashed anyway?

sever-sever · 2023-09-11T16:10:06Z

This check fails: "Check pull request message format / Check pull request title (pull_request)" i'm not sure how to fix the pull request message without redoing all my commits. Will the commits not be squashed anyway?

The commit message should include the task ID , for example, Txxx: Add some feature
Your commit messages don't include task ID

Apachez- · 2023-09-12T13:45:53Z

Note that command = command.lstrip() for def cmd in python/vyos/utils/process.py was reverted yesterday.

Causes funny problems during smoketests.

python/vyos/remote.py

interface-definitions/system-config-mgmt.xml.in

github-actions · 2023-10-09T17:30:06Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

sever-sever · 2023-10-15T13:54:51Z

Could you solve conflicts?

github-actions · 2023-10-15T17:03:48Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

yunzheng · 2023-10-15T17:05:45Z

Could you solve conflicts?

Done!

sever-sever · 2023-10-15T18:03:51Z

@jestabro could you take a look ?

c-po · 2023-10-16T18:53:12Z

Could we drop the current support/implementation for: /config/auth/commit-archive.key? This is a general topic not only for Git but also for SCP and SFTP commit-archives. We should find a common solution to this problem and not an individual one for Git only

yunzheng · 2023-10-17T09:49:57Z

Could this not be a common solution for both SCP and SFTP, as they are both SSH based?

On the other hand, i've looked into using the pki config management before I settled on this. However, I didn't like the same private key ending up in the repository that it's going to archive to.
IMHO private keys should remain private, if we could mark certain keys as "non exportable" that would be much nicer.

I can go ahead and remove /config/auth/commit-archive.key for now so we can solve this issue in another PR.
Let me know what the best solution forward would be for this PR.

c-po · 2023-11-01T08:04:20Z

I can go ahead and remove /config/auth/commit-archive.key for now so we can solve this issue in another PR.
Let me know what the best solution forward would be for this PR.

Please do so and remove /config/auth/commit-archive.key

While testing I've seen this:

cpo@LR1.wue3# set system config-management commit-archive location https://cpo:TOKEN@git.foo.de/cpo/vyos-git
[edit]
cpo@LR1.wue3# commit
Archiving config...
  https://cpo:TOKEN@git.foo.de/cpo/vyos-git OK
[edit]

Is there a way to prevent TOKEN to be echoed on the CLI?

Also my remote Git repo does not list any file(s) after adding it to VyOS. I'm using GitLab as backend service

yunzheng · 2023-11-10T21:53:04Z

Please do so and remove /config/auth/commit-archive.key

Removed in latest commit

While testing I've seen this:

cpo@LR1.wue3# set system config-management commit-archive location https://cpo:TOKEN@git.foo.de/cpo/vyos-git
[edit]
cpo@LR1.wue3# commit
Archiving config...
  https://cpo:TOKEN@git.foo.de/cpo/vyos-git OK
[edit]

Is there a way to prevent TOKEN to be echoed on the CLI?

The username and password are now removed from the output.

Also my remote Git repo does not list any file(s) after adding it to VyOS. I'm using GitLab as backend service

It looks like you used https instead of git+https. So it went to the HttpC handler instead of the GitC.

github-actions · 2023-11-16T21:54:59Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2023-11-16T22:09:26Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2023-11-16T22:21:28Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

c-po · 2023-11-17T06:47:01Z

It looks like you used https instead of git+https. So it went to the HttpC handler instead of the GitC.

Thanks for the hint!

c-po · 2023-11-17T06:53:13Z

Could this not be a common solution for both SCP and SFTP, as they are both SSH based?

On the other hand, i've looked into using the pki config management before I settled on this. However, I didn't like the same private key ending up in the repository that it's going to archive to. IMHO private keys should remain private, if we could mark certain keys as "non exportable" that would be much nicer.

I can go ahead and remove /config/auth/commit-archive.key for now so we can solve this issue in another PR. Let me know what the best solution forward would be for this PR.

Please go ahead and remove the hardcoded /config/auth/commit-archive.key path.
Please use PKI subsystem to supply key:

set pki key-pair GitHub private key ...
set pki key-pair GitHub public key ...

And select the key using: set system config-management commit-archive certificate, CLI node can be consumed via #include <include/pki/certificate-key.xml.i>

@sarthurdev what you think?

sarthurdev · 2023-11-17T11:23:37Z

Please go ahead and remove the hardcoded /config/auth/commit-archive.key path. Please use PKI subsystem to supply key:
* `set pki key-pair GitHub private key ...`

* `set pki key-pair GitHub public key ...`
And select the key using: set system config-management commit-archive certificate, CLI node can be consumed via #include <include/pki/certificate-key.xml.i>

@sarthurdev what you think?

Seems a good idea to me. Though you'd need to use include/pki/private-key.xml.i for the correct node.

yunzheng · 2023-11-17T23:16:45Z

Note that I already removed the hardcoded path /config/auth/commit-archive.key in a commit.

With that said, I'm looking into implementing set system config-management commit-archive private key, however, i'm running into usability issues. For example:

vyos@vyos# run generate pki ssh-key install my-key

This actually doesn't add any SSH-keys to PKI config.. only generate private key that you need to copy yourself and a pub key that is added to the current user for SSH access. (generated authorized_keys file)

Next, for the use case that you generate the SSH private key in VyOS itself:

vyos@vyos# run generate pki key-pair install my-key
Enter private key type: [rsa, dsa, ec] (Default: rsa)
Enter private key bits: (Default: 2048)
Note: If you plan to use the generated key on this router, do not encrypt the private key.
Do you want to encrypt the private key with a passphrase? [y/N]
Do you want to install the public key? [Y/n]
Do you want to install the private key? [Y/n]
2 value(s) installed. Use "compare" to see the pending changes, and "commit" to apply.

This generate a private and public key pair, but these are not directly OpenSSH compatible. For example the public base64 string is not compatible with OpenSSH format that all Git systems use.

Would it not be very confusing and annoying for users if they still need to convert this base64 string to a proper OpenSSH public key?

Also I think it's still a security risk to export the full VyOS config with the same private key that is also used for Git write access. Should this not be of concern? Or is possible to mark a PKI key as non exportable?

c-po · 2023-11-18T19:41:37Z

@sarthurdev I think we should add an option to generate SSH keys as they seem not to match regular key-pairs and add that to the PKI tree?

debian/control

python/vyos/config_mgmt.py

python/vyos/remote.py

sarthurdev · 2023-11-18T20:12:20Z

@sarthurdev I think we should add an option to generate SSH keys as they seem not to match regular key-pairs and add that to the PKI tree?

Sure I'll look into it. It should be trivial to implement, just need to serialize differently for OpenSSH.

I am however in agreement with @yunzheng regarding the security issue. We should look into a way to exclude (or perhaps encrypt) certain nodes/values from the archive.

c-po · 2023-11-18T20:23:22Z

We can also merge this first and only work with git+https to get some "feedback" and add additional features later on. Please note there is a bug in debian/control file

c-po · 2023-11-18T20:30:27Z

cpo@LR1.wue3# commit comment "foo bat"
Archiving config...
  git+https://git.foo.de/cpo/vyos-git.git OK
[edit]

Latest version looks good to me

sever-sever · 2023-11-18T21:23:18Z

I want to get choose if I want use PKI or files for all certificates/keys mentions.
And it is not only security reasons and maybe more holywar.

c-po · 2023-11-19T08:13:36Z

@Mergifyio backport sagitta

mergify · 2023-11-19T08:13:41Z

backport sagitta

✅ Backports have been created

#2505 T2405: add Git support to commit-archive (backport #2241) has been created for branch sagitta but encountered conflicts

T2405: add Git support to commit-archive (backport #2241)

github-actions bot added the current label Sep 11, 2023

vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team September 11, 2023 15:51

github-actions bot assigned yunzheng Sep 11, 2023

yunzheng force-pushed the feature/commit-archive-git-backend branch from 009eeb6 to 0b8c578 Compare September 11, 2023 16:52

sever-sever reviewed Sep 12, 2023

View reviewed changes

python/vyos/remote.py Show resolved Hide resolved

dmbaturin requested changes Sep 14, 2023

View reviewed changes

interface-definitions/system-config-mgmt.xml.in Show resolved Hide resolved

yunzheng requested a review from dmbaturin September 23, 2023 09:26

github-actions bot added the state: conflict label Oct 9, 2023

yunzheng force-pushed the feature/commit-archive-git-backend branch from 0d14dd2 to 52a8f54 Compare October 15, 2023 17:03

github-actions bot removed the state: conflict label Oct 15, 2023

sever-sever approved these changes Oct 15, 2023

View reviewed changes

github-actions bot removed the state: conflict label Nov 16, 2023

yunzheng force-pushed the feature/commit-archive-git-backend branch from 4a5ed21 to e36d04b Compare November 16, 2023 22:09

github-actions bot added the state: conflict label Nov 16, 2023

yunzheng force-pushed the feature/commit-archive-git-backend branch from e36d04b to eb8842c Compare November 16, 2023 22:12

yunzheng added 5 commits November 16, 2023 22:19

T2405: add Git support to commit-archive

4beebac

T2405: revert command.lstrip()

620b5da

T2405: Add regex for git and ssh protocol

754390f

T2405: Redact username and password

d09ed94

T2405: Remove commit-archive.key support

529216a

yunzheng force-pushed the feature/commit-archive-git-backend branch from eb8842c to 529216a Compare November 16, 2023 22:21

github-actions bot removed the state: conflict label Nov 16, 2023

c-po requested changes Nov 18, 2023

View reviewed changes

debian/control Show resolved Hide resolved

python/vyos/config_mgmt.py Show resolved Hide resolved

python/vyos/remote.py Show resolved Hide resolved

c-po approved these changes Nov 18, 2023

View reviewed changes

c-po merged commit a89243c into vyos:current Nov 18, 2023
7 checks passed

mergify bot mentioned this pull request Nov 19, 2023

T2405: add Git support to commit-archive (backport #2241) #2505

Merged

c-po added a commit that referenced this pull request Nov 20, 2023

Merge pull request #2505 from vyos/mergify/bp/sagitta/pr-2241

293f6b2

T2405: add Git support to commit-archive (backport #2241)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T2405: add Git support to commit-archive #2241

T2405: add Git support to commit-archive #2241

yunzheng commented Sep 11, 2023

yunzheng commented Sep 11, 2023 •

edited

sever-sever commented Sep 11, 2023

Apachez- commented Sep 12, 2023

github-actions bot commented Oct 9, 2023

sever-sever commented Oct 15, 2023

github-actions bot commented Oct 15, 2023

yunzheng commented Oct 15, 2023

sever-sever commented Oct 15, 2023

c-po commented Oct 16, 2023

yunzheng commented Oct 17, 2023

c-po commented Nov 1, 2023 •

edited

yunzheng commented Nov 10, 2023

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

c-po commented Nov 17, 2023

c-po commented Nov 17, 2023

sarthurdev commented Nov 17, 2023

yunzheng commented Nov 17, 2023 •

edited

c-po commented Nov 18, 2023

sarthurdev commented Nov 18, 2023

c-po commented Nov 18, 2023 •

edited

c-po commented Nov 18, 2023 •

edited

sever-sever commented Nov 18, 2023

c-po commented Nov 19, 2023

mergify bot commented Nov 19, 2023 •

edited

T2405: add Git support to commit-archive #2241

T2405: add Git support to commit-archive #2241

Conversation

yunzheng commented Sep 11, 2023

Change Summary

Types of changes

Related Task(s)

Component(s) name

Proposed changes

How to test

Checklist:

yunzheng commented Sep 11, 2023 • edited

sever-sever commented Sep 11, 2023

Apachez- commented Sep 12, 2023

github-actions bot commented Oct 9, 2023

sever-sever commented Oct 15, 2023

github-actions bot commented Oct 15, 2023

yunzheng commented Oct 15, 2023

sever-sever commented Oct 15, 2023

c-po commented Oct 16, 2023

yunzheng commented Oct 17, 2023

c-po commented Nov 1, 2023 • edited

yunzheng commented Nov 10, 2023

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

c-po commented Nov 17, 2023

c-po commented Nov 17, 2023

sarthurdev commented Nov 17, 2023

yunzheng commented Nov 17, 2023 • edited

c-po commented Nov 18, 2023

sarthurdev commented Nov 18, 2023

c-po commented Nov 18, 2023 • edited

c-po commented Nov 18, 2023 • edited

sever-sever commented Nov 18, 2023

c-po commented Nov 19, 2023

mergify bot commented Nov 19, 2023 • edited

✅ Backports have been created

yunzheng commented Sep 11, 2023 •

edited

c-po commented Nov 1, 2023 •

edited

yunzheng commented Nov 17, 2023 •

edited

c-po commented Nov 18, 2023 •

edited

c-po commented Nov 18, 2023 •

edited

mergify bot commented Nov 19, 2023 •

edited