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

enable to specify agent connection to insert cert to #231

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

ph4r05
Copy link
Collaborator

@ph4r05 ph4r05 commented May 25, 2024

Required for PS to create ephemeral agents to work around limitation on number of auth attempts. For each session we will create a new agent so users can have multiple simultaneous sessions.

@ph4r05 ph4r05 requested review from cviecco and rgooch May 25, 2024 16:26
Copy link

codecov bot commented May 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 32.13%. Comparing base (413cb76) to head (c9d9492).

Files Patch % Lines
lib/client/sshagent/agent.go 0.00% 13 Missing ⚠️
lib/client/sshagent/api.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
- Coverage   32.39%   32.13%   -0.26%     
==========================================
  Files          75       75              
  Lines        9718     9730      +12     
==========================================
- Hits         3148     3127      -21     
- Misses       5929     5962      +33     
  Partials      641      641              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ph4r05 ph4r05 force-pushed the pr branch 2 times, most recently from 0b5a325 to 7a76147 Compare May 25, 2024 17:11
Comment on lines 94 to 98
var err error = nil
if conn == nil {
conn, err = connectToDefaultSSHAgentLocation()
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Put the connectToDefaultSSHAgentLocation inside withAddedKeyUpsertCertIntoAgent. In general if we need an input parameter we dont change its lifetime within a function (as written sometimes the a connection is closed and sometimes is not).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, pls check now

@@ -50,12 +50,13 @@ func deleteDuplicateEntries(comment string, agentClient agent.ExtendedAgent, log
return deletedCount, nil
}

func upsertCertIntoAgent(
func upsertCertIntoAgentConnection(
Copy link
Contributor

Choose a reason for hiding this comment

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

None of the api (public calls are being modified here). What calls do you actually need? Looking a the keymaster code, it seems like The public side of this call is not needed.

Copy link
Collaborator Author

@ph4r05 ph4r05 May 28, 2024

Choose a reason for hiding this comment

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

Purelogin calls UpsertCertIntoAgent from [email protected]/lib/client/sshagent/api.go , I need to be able to specify connection there

comment string,
lifeTimeSecs uint32,
confirmBeforeUse bool,
logger log.DebugLogger) error {
Copy link
Contributor

@cviecco cviecco May 27, 2024

Choose a reason for hiding this comment

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

creation of the new agent connection should also go here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, pls check now

@ph4r05
Copy link
Collaborator Author

ph4r05 commented Jun 3, 2024

I've surfaced new methods to API, pls check it out :)

Dušan Klinec added 2 commits June 3, 2024 16:18
Copy link
Contributor

@cviecco cviecco left a comment

Choose a reason for hiding this comment

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

There is an unnecesary api and we should have more tests. But will approve.

@cviecco cviecco merged commit efb227d into Cloud-Foundations:master Jun 3, 2024
7 of 9 checks passed
@ph4r05 ph4r05 deleted the pr branch June 4, 2024 10:55
ph4r05 added a commit to ph4r05/keymaster that referenced this pull request Jun 12, 2024
* add flavour, version command, fix version source (Cloud-Foundations#229)

- make makefile single source of truth for version
- trigger the flow in the tests

* minor tests enhancements (Cloud-Foundations#232)

* Docker cleanup (Cloud-Foundations#233)

* Removed unnecessary `start.sh`
* Updated Dockerfile to newer OS
* Cleaned up Dockerfile dirty hack for RSA keys

Co-authored-by: Espinoza, Erik <[email protected]>

* enable to specify agent connection to insert cert to (Cloud-Foundations#231)

* enable to specify agent connection to insert cert to

* add api

* bump version

---------

Co-authored-by: Dušan Klinec <[email protected]>

---------

Co-authored-by: Dušan Klinec <[email protected]>
Co-authored-by: cviecco <[email protected]>
Co-authored-by: Erik Espinoza <[email protected]>
Co-authored-by: Espinoza, Erik <[email protected]>
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.

2 participants