borgmatic-collective/borgmatic
10
71
Fork 42
Code Issues 49 Pull Requests 6 Actions 1 Packages Releases 153 Activity

Fully support keepassxc-cli options #1047 #1049

Merged
witten merged 7 commits from gautamaggarwal2810/borgmatic:issue1047 into main 2025-04-03 18:28:08 +00:00
Conversation 11 Commits 7 Files Changed 3 +132 -20
gautamaggarwal2810 commented 2025-03-30 14:11:31 +00:00
Contributor
Copy Link

This is first draft for issue #1047. Please review and suggest changes if any.

This is first draft for issue #1047. Please review and suggest changes if any.
gautamaggarwal2810 added 1 commit 2025-03-30 14:11:32 +00:00
First Draft f7e4d38762
gautamaggarwal2810 reopened this pull request 2025-03-30 14:39:32 +00:00
gautamaggarwal2810 added 1 commit 2025-03-30 15:58:41 +00:00
added schema.yaml 863c954144
gautamaggarwal2810 added 1 commit 2025-03-30 16:20:40 +00:00
2nd Draft 92ebc77597
gautamaggarwal2810 commented 2025-03-30 16:37:28 +00:00
Author
Contributor
Copy Link

I am not sure if yubikey option should be implemented as boolean flag or not. But on googling,I found that when modeling YubiKey usage in a configuration file or schema that doesnt need device details, it might be represented as a boolean instead of string.However if that needs to change,please let me know.

I am not sure if yubikey option should be implemented as boolean flag or not. But on googling,I found that when modeling YubiKey usage in a configuration file or schema that doesnt need device details, it might be represented as a boolean instead of string.However if that needs to change,please let me know.
witten reviewed 2025-03-31 17:10:47 +00:00
witten left a comment
Owner
Copy Link

Thanks for doing this!

Thanks for doing this!
borgmatic/config/schema.yaml Outdated
@@ -2683,6 +2683,16 @@ properties:
description: |
Command to use instead of "keepassxc-cli".
example: /usr/local/bin/keepassxc-cli
key-file:
witten commented 2025-03-31 17:02:57 +00:00
Owner
Copy Link

Underscore instead of dash here. (Convention for option names.)

Underscore instead of dash here. (Convention for option names.)
gautamaggarwal2810 marked this conversation as resolved
borgmatic/config/schema.yaml Outdated
@@ -2686,0 +2689,4 @@
Path to a key file for unlocking the KeePassXC database.
example: /path/to/keyfile
yubikey:
type: boolean
witten commented 2025-03-31 17:04:28 +00:00
Owner
Copy Link

If you look at the help for keepassxc-cli show, this looks like it should be a string rather than a bool.

If you look at the help for `keepassxc-cli show`, this looks like it should be a string rather than a bool.
gautamaggarwal2810 marked this conversation as resolved
borgmatic/hooks/credential/keepassxc.py Outdated
@@ -18,2 +18,3 @@
try:
(database_path, attribute_name) = credential_parameters
database_path, attribute_name = credential_parameters[:2]
extra_args = credential_parameters[2:] # Handle additional arguments like --key-file or --yubikey
witten commented 2025-03-31 17:06:33 +00:00
Owner
Copy Link

I don't think this should be necessary since you've got key file and yubikey option in config. If you want to support arbitrary extra arguments though, I'd recommend doing that with a dedicated config option for that purpose.

I don't think this should be necessary since you've got key file and yubikey option in config. If you want to support arbitrary extra arguments though, I'd recommend doing that with a dedicated config option for that purpose.
gautamaggarwal2810 marked this conversation as resolved
witten reviewed 2025-03-31 17:12:04 +00:00
borgmatic/hooks/credential/keepassxc.py Outdated
@@ -43,2 +39,3 @@
)
).rstrip(os.linesep)
+ tuple(extra_args) # Append extra arguments
)
witten commented 2025-03-31 17:12:04 +00:00
Owner
Copy Link

I might've missed it, but I don't see where your new config options are used here so they impact the constructed command.

I might've missed it, but I don't see where your new config options are used here so they impact the constructed command.
gautamaggarwal2810 marked this conversation as resolved
gautamaggarwal2810 added 1 commit 2025-04-02 10:48:13 +00:00
Applied changes 7a0c56878b
gautamaggarwal2810 added 1 commit 2025-04-02 10:50:52 +00:00
Applied changes 96ec66de79
gautamaggarwal2810 commented 2025-04-02 10:54:05 +00:00
Author
Contributor
Copy Link

Added all suggested changes.Please review.

Added all suggested changes.Please review.
aledeg reviewed 2025-04-02 13:56:16 +00:00
tests/unit/hooks/credential/test_keepassxc.py
@@ -119,0 +193,4 @@
'execute_command_and_capture_output'
).with_args(
(
'keepassxc-cli',
aledeg commented 2025-04-02 13:56:16 +00:00
First-time contributor
Copy Link

In the documentation, the database and the entry have to be defined at the end of the command:

Usage: keepassxc-cli show [options] database entry
Show an entry's information.

Options:
  -q, --quiet                    Silence password prompt and other secondary
                                 outputs.
  -k, --key-file <path>          Key file of the database.
  --no-password                  Deactivate password key for the database.
  -y, --yubikey <slot[:serial]>  Yubikey slot and optional serial used to
                                 access the database (e.g., 1:7370001).
  -t, --totp                     Show the entry's current TOTP.
  -a, --attributes <attribute>   Names of the attributes to show. This option
                                 can be specified more than once, with each
                                 attribute shown one-per-line in the given
                                 order. If no attributes are specified, a
                                 summary of the default attributes is given.
  -s, --show-protected           Show the protected attributes in clear text.
  --all                          Show all the attributes of the entry.
  --show-attachments             Show the attachments of the entry.
  -h, --help                     Display this help.

Arguments:
  database                       Path of the database.
  entry                          Name of the entry to show.

I've tested with the order you've generated. It's working. For now.
Nothing guarantees that the order will not be enforced later.
You might want to generate the command as it is described in the documentation.

In the documentation, the database and the entry have to be defined at the end of the command: ``` Usage: keepassxc-cli show [options] database entry Show an entry's information. Options: -q, --quiet Silence password prompt and other secondary outputs. -k, --key-file <path> Key file of the database. --no-password Deactivate password key for the database. -y, --yubikey <slot[:serial]> Yubikey slot and optional serial used to access the database (e.g., 1:7370001). -t, --totp Show the entry's current TOTP. -a, --attributes <attribute> Names of the attributes to show. This option can be specified more than once, with each attribute shown one-per-line in the given order. If no attributes are specified, a summary of the default attributes is given. -s, --show-protected Show the protected attributes in clear text. --all Show all the attributes of the entry. --show-attachments Show the attachments of the entry. -h, --help Display this help. Arguments: database Path of the database. entry Name of the entry to show. ``` I've tested with the order you've generated. It's working. For now. Nothing guarantees that the order will not be enforced later. You might want to generate the command as it is described in the documentation.
gautamaggarwal2810 commented 2025-04-02 14:12:11 +00:00
Author
Contributor
Copy Link

Ok I will get right to it !!

Ok I will get right to it !!
👍 1 ❤️ 1
gautamaggarwal2810 marked this conversation as resolved
gautamaggarwal2810 added 1 commit 2025-04-02 15:35:55 +00:00
Command Restructuring 4e55547235
gautamaggarwal2810 commented 2025-04-03 04:57:46 +00:00
Author
Contributor
Copy Link

The latest commit generates command as per documentation.All tests were successful.Please let me know if any more changes are required.

The latest commit generates command as per documentation.All tests were successful.Please let me know if any more changes are required.
🎉 1
witten reviewed 2025-04-03 15:48:34 +00:00
witten left a comment
Owner
Copy Link

A few minor notes, but I think this is just about done! Thank you!

A few minor notes, but I think this is just about done! Thank you!
borgmatic/config/schema.yaml Outdated
@@ -2686,0 +2691,4 @@
yubikey:
type: string
description: |
Path or identifier for the YubiKey to use for unlocking the KeePassXC database.
witten commented 2025-04-03 15:39:03 +00:00
Owner
Copy Link

I don't think this is quite right. If I'm reading the --help correctly, this isn't ever a path. Check out the keepassxc-cli show help for details.

I don't think this is quite right. If I'm reading the `--help` correctly, this isn't ever a path. Check out the `keepassxc-cli show` help for details.
borgmatic/hooks/credential/keepassxc.py Outdated
@@ -17,3 +17,3 @@
'''
try:
(database_path, attribute_name) = credential_parameters
database_path, attribute_name = credential_parameters[:2]
witten commented 2025-04-03 15:42:35 +00:00
Owner
Copy Link

Adding [:2] means that we'll no longer error if the user adds too many credential parameters; we'll just silently ignore extra ones. But it seems like erroring would be preferable...?

Let me know if you disagree.

Adding `[:2]` means that we'll no longer error if the user adds too many credential parameters; we'll just silently ignore extra ones. But it seems like erroring would be preferable...? Let me know if you disagree.
gautamaggarwal2810 commented 2025-04-03 17:15:53 +00:00
Author
Contributor
Copy Link

I think u are right. I have made changes accordingly in final commit.

I think u are right. I have made changes accordingly in final commit.
witten marked this conversation as resolved
borgmatic/hooks/credential/keepassxc.py
@@ -45,0 +33,4 @@
+ (('--key-file', hook_config['key_file']) if 'key_file' in hook_config else ())
+ (('--yubikey', hook_config['yubikey']) if 'yubikey' in hook_config else ())
+ (expanded_database_path, attribute_name) # Ensure database & entry are last
)
witten commented 2025-04-03 15:44:49 +00:00
Owner
Copy Link

Looks great!

Looks great!
witten marked this conversation as resolved
tests/unit/hooks/credential/test_keepassxc.py
@@ -119,0 +216,4 @@
credential_parameters=('database.kdbx', 'mypassword'),
)
== 'password'
)
witten commented 2025-04-03 15:47:52 +00:00
Owner
Copy Link

Nice tests!

I don't know that this last one is strictly necessary though. But I also don't feel strongly if you want to leave it.

Nice tests! I don't know that this last one is strictly necessary though. But I also don't feel strongly if you want to leave it.
gautamaggarwal2810 commented 2025-04-03 17:15:06 +00:00
Author
Contributor
Copy Link

Thank you. I have created some differentiation from earlier tests for the last test by testing only for slot in yubikey string value so that you might be inclined to accept it 😄

Thank you. I have created some differentiation from earlier tests for the last test by testing only for slot in yubikey string value so that you might be inclined to accept it 😄
witten marked this conversation as resolved
gautamaggarwal2810 added 1 commit 2025-04-03 17:11:31 +00:00
Final 248999c23e
witten commented 2025-04-03 18:03:42 +00:00
Owner
Copy Link

As far as I'm concerned, this is ready to merge! Please remove the "WIP" prefix when you're ready.

As far as I'm concerned, this is ready to merge! Please remove the "WIP" prefix when you're ready.
gautamaggarwal2810 changed title from WIP: Fully support keepassxc-cli options #1047 to Fully support keepassxc-cli options #1047 2025-04-03 18:22:55 +00:00
witten merged commit 1c9d25b892 into main 2025-04-03 18:28:08 +00:00
witten commented 2025-04-03 18:28:17 +00:00
Owner
Copy Link

Thank you for doing this work!

Thank you for doing this work!
witten reviewed 2025-04-03 18:44:53 +00:00
borgmatic/hooks/credential/keepassxc.py
@@ -43,2 +32,2 @@
)
).rstrip(os.linesep)
+ ('show', '--show-protected', '--attributes', 'Password')
+ (('--key-file', hook_config['key_file']) if 'key_file' in hook_config else ())
witten commented 2025-04-03 18:44:53 +00:00
Owner
Copy Link

FYI this errored when keepassxc: was completely omitted from configuration (which is a valid way to use the hook). So I changed it to first check that hook_config is set at all.

FYI this errored when `keepassxc:` was completely omitted from configuration (which is a valid way to use the hook). So I changed it to first check that `hook_config` is set at all.
witten reviewed 2025-04-03 18:45:55 +00:00
borgmatic/config/schema.yaml
@@ -2686,0 +2694,4 @@
YubiKey slot and optional serial number used to access the KeePassXC database.
Format: "<slot[:serial]>", where:
- <slot> is the YubiKey slot number (e.g., `1` or `2`).
- <serial> (optional) is the YubiKey's serial number (e.g., `1:7370001`).
witten commented 2025-04-03 18:45:55 +00:00
Owner
Copy Link

FYI some of these lines are too long and tests complained, so I word-wrapped the lines. In the future, please make sure you run the full tox suite of tests so it catches things like this for you. Thanks!

FYI some of these lines are too long and tests complained, so I word-wrapped the lines. In the future, please make sure you run the full `tox` suite of tests so it catches things like this for you. Thanks!
Sign in to join this conversation.
Reviewers
No Reviewers
aledeg
witten
Labels
Clear labels
blocked
breaking
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
witten
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#1049
Delete Branch "gautamaggarwal2810/borgmatic:issue1047"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?