-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add Identity support to dev environment #124
Conversation
861ee54
to
d3c6ac6
Compare
d3c6ac6
to
6d32796
Compare
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.
Looks good, just left a couple questions. Also looks like a rebase will fix the unit tests
ci/identity/users.template.yml
Outdated
# Users with permission to authenticate | ||
- !user test.user3@mycompany.com | ||
- !user conjur_ci_user@cyberark.com | ||
- !user conj_ops_dev@cyberark.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.
Just noticing that these hard-coded users are the same as Okta. That may be valid depending on infra's setup, but if we are also enforcing that IDENTITY_USERNAME must be set in the environment when running dev/start
, are these other users needed?
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.
Not at the moment - they might be introduced with future integration tests, but they can be added then. Removing.
echo "Setting up Conjur for OIDC (Identity)" | ||
docker-compose exec cli-dev bash -c 'conjur logout | ||
conjur init --force-netrc --force -u http://conjur -i -a dev -t oidc --service-id identity | ||
conjur login -i $IDENTITY_USERNAME' |
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.
Is it assumed that a developer will be using their own account as IDENTITY_USERNAME
and providing their own password?
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.
Yeah, that's the idea. Feel free to to test it out, all C&I team should have Identity access now.
return authRequest[advanceAuthData, advanceAuthResponse](endpoint, data, httpClient) | ||
} | ||
|
||
func fetchAuthTokenFromIdentity(httpClient *http.Client, providerURL string, username string, password string) (string, error) { |
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.
Nice job on this! Looks like Identity makes it tricky to automate these calls 😳
InnerExceptions string `json:"InnerExceptions"` | ||
} | ||
|
||
func authRequest[data startAuthData | advanceAuthData, responseContent startAuthResponse | advanceAuthResponse]( |
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.
Looks like CodeClimate is unable to lint the file due to this square bracket notation for type contraints. As far as I can tell its valid - any idea what that's about?
Could not lint file: /code/pkg/clients/authn_oidc_dev.go
Error /code/pkg/clients/authn_oidc_dev.go:291:17: expected '(', found '[' (and 2 more errors)
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.
It looks like golint
was frozen and deprecated in 2020, and Go introduced generics in 2022 with 1.18, so they definitely aren't supported. Maybe we should disable golint
in CodeClimate, it's bound to get less and less accurate.
6d32796
to
4c42aab
Compare
golint was frozen and deprecated in 2020. We should disable golint on this project, as we use Go 1.19, released in 2022. golint deprecation issue: golang/go#38968
4c42aab
to
03abeeb
Compare
Code Climate has analyzed commit 03abeeb and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 82.2% (0.0% change). View more on Code Climate. |
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.
LGTM!
Desired Outcome
This pull request adds support for Identity in the Dev Environment.
Implemented Changes
Describe how the desired outcome above has been achieved with this PR. In
particular, consider:
Definition of Done
At least 1 todo must be completed in the sections below for the PR to be
merged.
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
README
s) were updated in this PRBehavior
Security