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

Check for private key file with SDW config validator. Add validator unit tests. #1205

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

rocodes
Copy link
Contributor

@rocodes rocodes commented Nov 28, 2024

Status

Ready for review

Description of Changes

Fixes #1202

  • smoketest for armored private key files in sdw config validator
  • add sdw config validator unit tests

Fixes #1215

Testing

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing pilot instances
  2. New installs

Checklist

If you have made changes to the provisioning logic

  • All tests (make test) pass in dom0

If you have added or removed files

  • I have updated MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec

If documentation is required

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation

@rocodes rocodes force-pushed the 1202-validate-key-no-really branch from c23b9fd to dc7f3aa Compare November 28, 2024 20:48
@rocodes rocodes assigned rocodes and unassigned rocodes Nov 28, 2024
@rocodes rocodes force-pushed the 1202-validate-key-no-really branch 2 times, most recently from 454a729 to 2f74007 Compare November 28, 2024 21:12
@deeplow deeplow self-requested a review November 29, 2024 16:33
@deeplow deeplow self-assigned this Nov 29, 2024
deeplow
deeplow previously approved these changes Nov 29, 2024
Copy link
Contributor

@deeplow deeplow left a comment

Choose a reason for hiding this comment

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

Approved. Happy to see some guard-rails for potentially Johnny-can't-encrypt type problems :)

It's currently only missing CI, which failed for another reason.

@deeplow
Copy link
Contributor

deeplow commented Nov 29, 2024

It's an unrelated issue, but @rocodes what do you think about printing just the error message as opposed to the python traceback?

@rocodes
Copy link
Contributor Author

rocodes commented Dec 2, 2024

@deeplow I'm generally in favour of that :) Are you referring to the CI failure here:
INFO:[2024-11-28-21:43:37:519991] subprocess.CalledProcessError: Command '['sudo', 'qubesctl', '--show-output', 'state.highstate']' returned non-zero exit status 1. [...] ? If yes, I'd suggest that as a separate issue about console output during provisioning, which I agree would be an improvement.

@deeplow
Copy link
Contributor

deeplow commented Dec 2, 2024

Are you referring to the CI failure here:
INFO:[2024-11-28-21:43:37:519991] subprocess.CalledProcessError: Command '['sudo', 'qubesctl', '--show-output', 'state.highstate']' returned non-zero exit status 1. [...] ? If yes, I'd suggest that as a separate issue about console output during provisioning, which I agree would be an improvement.

Sorry, but I'm not sure I follow. I'm talking about that issue, but it does show the salt output. The error is:


INFO:[2024-11-28-21:43:37:490899]           ID: enable-user-units
INFO:[2024-11-28-21:43:37:490945]     Function: cmd.run
INFO:[2024-11-28-21:43:37:490974]         Name: systemctl --user daemon-reload
INFO:[2024-11-28-21:43:37:491006] systemctl --user enable sdw-notify.timer
INFO:[2024-11-28-21:43:37:491034] 
INFO:[2024-11-28-21:43:37:491066]       Result: False
INFO:[2024-11-28-21:43:37:491093]      Comment: Command "systemctl --user daemon-reload
INFO:[2024-11-28-21:43:37:491126]               systemctl --user enable sdw-notify.timer
INFO:[2024-11-28-21:43:37:491151]               " run
INFO:[2024-11-28-21:43:37:491184]      Started: 21:42:53.059047
INFO:[2024-11-28-21:43:37:491210]     Duration: 189.213 ms
INFO:[2024-11-28-21:43:37:491243]      Changes:   
INFO:[2024-11-28-21:43:37:491269]               ----------
INFO:[2024-11-28-21:43:37:491302]               pid:
INFO:[2024-11-28-21:43:37:491328]                   13529
INFO:[2024-11-28-21:43:37:491360]               retcode:
INFO:[2024-11-28-21:43:37:491386]                   1
INFO:[2024-11-28-21:43:37:491418]               stderr:
INFO:[2024-11-28-21:43:37:491444]                   Failed to connect to bus: No such file or directory
INFO:[2024-11-28-21:43:37:491477]                   Failed to connect to bus: No such file or directory
INFO:[2024-11-28-21:43:37:491503]               stdout:

@rocodes rocodes force-pushed the 1202-validate-key-no-really branch from 2f74007 to 90e849d Compare December 2, 2024 18:42
@rocodes
Copy link
Contributor Author

rocodes commented Dec 2, 2024

(Just rebased to try to get a successful CI run.)

@rocodes
Copy link
Contributor Author

rocodes commented Dec 4, 2024

Sorry, but I'm not sure I follow. I'm talking about that issue, but it does show the salt output. The error is: [snip]

Yes; it looks like CI has been failing on main for a while, I'm looking into it, but it's not code that is introduced in this PR. I will investigate and hopefully put up a fix today, then we can see some more 🟢 here. : )

Regarding your request about not showing tracebacks, I am in favour, and since that's also a bit outside the scope of this specific PR I have filed #1207 - hth :)

@rocodes rocodes force-pushed the 1202-validate-key-no-really branch from 18b1efa to f23af06 Compare December 6, 2024 21:46
@rocodes rocodes requested a review from a team December 6, 2024 21:49
@rocodes
Copy link
Contributor Author

rocodes commented Dec 6, 2024

(CI issue should be resolved; now also includes a fix for #1215 and an updated test plan item)

@rocodes rocodes requested a review from deeplow December 9, 2024 14:12
Copy link
Contributor

@deeplow deeplow left a comment

Choose a reason for hiding this comment

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

New changes look good. Approved for your merge, @rocodes.

@rocodes
Copy link
Contributor Author

rocodes commented Dec 11, 2024

Thank you!

@rocodes rocodes added this pull request to the merge queue Dec 11, 2024
Merged via the queue into main with commit 467ac3f Dec 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
2 participants