-
Notifications
You must be signed in to change notification settings - Fork 4
Major update of the code #1
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added comments in-line for all the changes requested.
I will note that while trying to test this in my lab, I was unable to Initialize Conjur using the example on the README.
After changes are made and example code on the README is fixed, I will be able to run tests on all functions and provide further review.
CyberarkConjur.psm1
Outdated
$CCConfig = @{ | ||
AWS_MetaData = "169.254.169.254" | ||
Account = $null | ||
AuthaurityName = $null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be AuthorityName
CyberarkConjur.psm1
Outdated
AWS_MetaData = "169.254.169.254" | ||
Account = $null | ||
AuthaurityName = $null | ||
AuthaurityName_WR = $null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be AuthorityName_WR
CyberarkConjur.psm1
Outdated
$responseBody = $null | ||
<# | ||
.SYNOPSIS | ||
This is the main fonction that does all the API calls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be function
CyberarkConjur.psm1
Outdated
return $responseBody; | ||
} | ||
.PARAMETER API | ||
[Mandatory] the main API branch you are calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide an example for further clarity.
CyberarkConjur.psm1
Outdated
return $res | ||
} | ||
.PARAMETER FixUri | ||
Fixing URI issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide a better description of what is being fixed in regards to "URI issues".
README.md
Outdated
#### Conjur authentication | ||
Prior to launching any commands, you will need to configure your conjur environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize conjur
and end with :
README.md
Outdated
PS C:\> $env:CONJUR_AUTHN_API_KEY="adminPassword" | ||
PS C:\> $env:CONJUR_APPLIANCE_URL="https://conjur.yourorg.com:443" | ||
PS C:\> $PsCredential = Get-Credential -Message "CyberArk Conjur Credential input" -UserName "host\Host_Identifier" | ||
PS C:\> Initialize-Conjur -Account Account -AuthnLogin "Identifier of a host" -Crednetial $PsCredential |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be -Credential
Also, I used this command to test the PR code and it failed with the following error:
PS C:\Users\jgarcia\Downloads\conjur-api-powershell> Initialize-Conjur -Account "cyberarkdemo" -AuthnLogin "https://conj
ur.joegarcia.dev" -Credential $PsCredential
Initialize-Conjur : Parameter set cannot be resolved using the specified named parameters.
At line:1 char:1
+ Initialize-Conjur -Account "cyberarkdemo" -AuthnLogin "https://conjur ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : InvalidArgument: (:) [Initialize-Conjur], ParameterBindingException
+ FullyQualifiedErrorId : AmbiguousParameterSet,Initialize-Conjur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until this is resolved, I cannot continue testing functions provided in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, there is some mistake here, you need to use this :
Initialize-Conjur -Account "cyberarkdemo" -AuthaurityName"conjur.joegarcia.dev" -Credential $PsCredential
Changed the readme again to bring more explenations
README.md
Outdated
PS C:\> $env:CONJUR_AUTHN_LOGIN="host/cust-portal/622703825757/ubuntu-client-conjur-identity" | ||
PS C:\> $env:CONJUR_IAM_AUTHN_BRANCH="authnBranchName" | ||
PS C:\> $env:CONJUR_APPLIANCE_URL="https://conjur.yourorg.com:443" | ||
PS C:\> Initialize-Conjur -Account Account -AuthnLogin "Identifier of a host" -AuthnApiKey "Your API generated key" -AuthaurityName "your-conjur-auth-read.mycompany.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be -AuthorityName
after corrections are made to CyberarkConjur.psm1
README.md
Outdated
``` | ||
|
||
#### IAM Authentication | ||
Some code has been started to be written, but is not good enough to publish anything yet. | ||
This would require someone with an IAM access to continue developping this code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be developing
and end with .
README.md
Outdated
#### Initialize-Conjur | ||
|
||
```powershell | ||
PS C:\> Initialize-Conjur -Account Account -AuthnLogin "Identifier of a host" -AuthnApiKey "Your API generated key" -AuthaurityName "your-conjur-auth-read.mycompany.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be -AuthorityName
after changes are made to CyberarkConjur.psm1
Just a gently nudge that there are still pending requested changes in order to merge this PR. 😄 |
Hello @infamousjoeg ... I might be wrong, but I really think I fixed all of your issues.
The issue does not come from the module but from how you are using the command. Initialize-Conjur can work in 2 different ways, and you can not mix them (different parameter set)
You can not mix This is also shown under the
|
Desired Outcome
Implemented Changes
Changed the way Conjur is beeing configured. The API key is now stored in a PsCredential object, which is much more secured.
Simplified all the functions code, which brought more complication in the code calling the API
Added some new functions
Added some new switches
could not check anything related to AWS, as I don't have access to such implementation. All AWS code would need to be tested.
Connected Issue/Story
Resolves my own requirements
Definition of Done
Changelog
Test coverage
No test
Documentation
README was updated
Inline help was updated
Behavior
Security