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

use ParseAuthorizedKey instead of string parsing of the public key #411

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

Xeckt
Copy link
Contributor

@Xeckt Xeckt commented Jun 21, 2024

Few changes in this pull request, more than anticipated:

  • Remove publickey struct and use only ssh.publickey interface
  • Taking advantage of the ssh.PublicKey Type() func to get a keys exact type for the publicKey struct:
  • Removed the len check on how many fields were detected in the string, ParseAuthorizedKey func will detect if it's a valid key or not
  • Changed the createtempfile func for testing to be more inclusive of it's intention, moved the dhcp string for testing to a constant in the daemon_test.go file
  • moved ssh logic into it's own file with tests
  • added helper functions for getting ssh strings

I hope all these changes are okay. Let me know if you would like to change anything or revert any of the changes. All tests have passed locally, including daemon tests.

@Xeckt Xeckt requested a review from a team as a code owner June 21, 2024 14:55
@Xeckt Xeckt changed the title closes #406 closes 406 Jun 21, 2024
@Xeckt Xeckt changed the title closes 406 closes https://github.com/opiproject/sztp/issues/406 Jun 21, 2024
@Xeckt Xeckt changed the title closes https://github.com/opiproject/sztp/issues/406 closes #406 Jun 21, 2024
@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 21, 2024

Sorry for the title changes I borked up some stuff :D

On second consideration, it might be better to keep the struct field I changed to type as Algorithm

Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

Thanks @Xeckt for the contribution
This is exactly the direction I was looking for
Few small comments inside, once fixed, we can merge this PR

sztp-agent/pkg/secureagent/utils.go Outdated Show resolved Hide resolved
sztp-agent/pkg/secureagent/utils.go Outdated Show resolved Hide resolved
sztp-agent/pkg/secureagent/utils.go Outdated Show resolved Hide resolved
sztp-agent/pkg/secureagent/daemon_test.go Outdated Show resolved Hide resolved
sztp-agent/pkg/secureagent/utils.go Outdated Show resolved Hide resolved
@glimchb glimchb linked an issue Jun 22, 2024 that may be closed by this pull request
@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 22, 2024

Thanks for the feedback! Will make the changes later today!

@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 24, 2024

All changes and feedback resolved. Let me know if the new changes are ok!

Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

looks great, thansk @Xeckt for contributing and fixing all issues
please add copyright in the new file and fix few ci failures we good to merge
at this point you can also squash all commit into a single one, review is done

@glimchb glimchb closed this Jun 24, 2024
@glimchb glimchb reopened this Jun 24, 2024
@glimchb glimchb changed the title closes #406 use ParseAuthorizedKey instead of string parsing of the public key Jun 24, 2024
@glimchb glimchb added the Merge Candidate in the open merge window, next candidate for merge label Jun 24, 2024
@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 24, 2024

Linter issues fixed, ran golangci-lint locally, seems all good.

@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 24, 2024

Commits squashed!

@glimchb
Copy link
Member

glimchb commented Jun 24, 2024

@Xeckt please note https://commitlint.js.org/ failing

@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 24, 2024

Gah! Sorry!

@glimchb
Copy link
Member

glimchb commented Jun 24, 2024

Gah! Sorry!

no worries. just add fix: or feat: in the start of the commit message and keep it lowercase...

Signed-off-by: Xeckt <jordansavell1198@gmail.com>
@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 24, 2024

Should be done now

@glimchb glimchb merged commit a9e5b9a into opiproject:main Jun 24, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Candidate in the open merge window, next candidate for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

agent: use ParseAuthorizedKey when parsing public key
2 participants