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

feat(api): add AuthorizedKeys reset option for the OT-2 + fixes #13745

Merged
merged 3 commits into from
Oct 10, 2023

Conversation

vegano1
Copy link
Contributor

@vegano1 vegano1 commented Oct 9, 2023

Overview

This change should have been merged into the chore_release-7.0.1 branch but was merged to edge instead. So let's Cherry-pick the commit from edge and retarget to 7.0.1 release branch. Original pr can be found here. This pr also adds a few improvements to the /from_keys function, we now skip hidden (.) files, we handle errors so we don't stop processing all pub keys found if one of them fails to open, we now accept ecdsa keys like the OT-2.

Test Plan

  • Make sure we can still use the ssh_keys/from_local endpoint to add ssh keys from a USB drive
  • Clear all ssh keys on the robot, make sure you can't ssh, now use the ssh_keys/from_local endpoint and make sure you can ssh.
  • Create a binary file that ends in .pub and make sure we skip it
  • Make sure we skip hidden files because MacOS likes to create hidden binary files that are invalid to us
  • Make sure we don't stop processing all valid public keys we find if one of them is invalid.

Changelog

  • Skip hidden files when searching the USB drive, MacOS likes to create hidden binary files in the USB drive
  • Added ecdsa as a valid public key type to match the OT-2
  • Handle exceptions so only skip the invalid key and not all subsequent potentially valid keys.

Review requests

Risk assessment

@vegano1 vegano1 requested review from a team as code owners October 9, 2023 18:16
@vegano1 vegano1 requested review from mjhuff and sfoster1 October 9, 2023 18:17
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #13745 (f712376) into chore_release-7.0.1 (9342034) will decrease coverage by 0.01%.
Report is 2 commits behind head on chore_release-7.0.1.
The diff coverage is 0.00%.

❗ Current head f712376 differs from pull request most recent head f4177de. Consider uploading reports for the commit f4177de to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.1   #13745      +/-   ##
=======================================================
- Coverage                71.24%   71.24%   -0.01%     
=======================================================
  Files                     2429     2429              
  Lines                    68430    68434       +4     
  Branches                  8040     8040              
=======================================================
  Hits                     48755    48755              
- Misses                   17776    17780       +4     
  Partials                  1899     1899              
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
update-server 63.51% <0.00%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
api/src/opentrons/config/reset.py 88.88% <ø> (ø)
...pdate-server/otupdate/common/ssh_key_management.py 65.04% <0.00%> (-2.19%) ⬇️

skip hidden files when searching for public keys
accept ecdsa keys as well
@vegano1 vegano1 changed the title feat(api): add AuthorizedKeys reset option for the OT-2 feat(api): add AuthorizedKeys reset option for the OT-2 + fixes Oct 10, 2023
@vegano1 vegano1 requested a review from mjhuff October 10, 2023 13:55
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

The changes to handling keys look good to me!

Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

Tested on an OT-2 and works! Code makes sense per in-person convo.

report that keys were added if they are valid even if they already exist on the bot, so they get feedback on the operation.
@vegano1 vegano1 merged commit 6b2b70e into chore_release-7.0.1 Oct 10, 2023
19 of 24 checks passed
@vegano1 vegano1 deleted the add-reset-ssh-authorized-keys-ot2 branch October 10, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants