Enable gcp authenticator #2448

aloncarmel111 · 2021-12-20T08:20:21Z

Desired Outcome

I want to be able to enable GCP authenticator on Conjur, so I could authenticate using authn-gcp authenticator, to authenticate securely on GCP platform.

Currently to enable authenticator, the user needs to run REST API request to enable the authenticator. The REST doesn't support authenticator that doesn't have serviceid.

We should add the ability to enable authenticator that missing service-id.

Implemented Changes

We want to use the existing REST API endpoint, and update service-id to be optional parameter in the URL
Before change:
patch '/:authenticator/:service_id/:account' => 'authenticate#update_config'

After change:
patch '/:authenticator/(:service_id)/:account' => 'authenticate#update_config'

Connected Issue/Story

Resolves #2389

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a
CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code
changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR
A follow-up issue to update official docs has been filed here: insert issue ID
This PR does not require updating any documentation

Behavior

This PR changes product behavior and has been reviewed by a PO, or
These changes are part of a larger initiative that will be reviewed later, or
No behavior was changed with this PR

Security

Security architect has reviewed the changes in this PR,
These changes are part of a larger initiative with a separate security review, or
There are no security aspects to these changes

app/domain/authentication/security/validate_webservice_is_authenticator.rb

CHANGELOG.md

tzheleznyak · 2021-12-20T09:37:44Z

app/domain/authentication/security/validate_webservice_is_authenticator.rb

@@ -21,7 +21,7 @@ def validate_webservice_is_configured_authenticator
      end

      def webservice_id
-        "#{@webservice.authenticator_name}/#{@webservice.service_id}"
+        @webservice.service_id.blank? ? @webservice.authenticator_name : "#{@webservice.authenticator_name}/#{@webservice.service_id}"


Maybe in this place you should call function that checks if the authenticator is with mandatory for service id?

tzheleznyak

LGTM

nessiLahav · 2021-12-21T09:42:26Z

config/routes.rb

@@ -25,7 +25,7 @@ def matches?(request)
        get '/authn-jwt/:service_id/:account/status' => 'authenticate#authn_jwt_status'
        get '/:authenticator(/:service_id)/:account/status' => 'authenticate#status'

-        patch '/:authenticator/:service_id/:account' => 'authenticate#update_config'
+        patch '/:authenticator(/:service_id)/:account' => 'authenticate#update_config'


WDYT on the following change?

patch '/:authn-gcp/:account' => 'authenticate#update_config' patch '/:authenticator/:service_id/:account' => 'authenticate#update_config'

Think that there are some other authenticator define with service-id as an optional parameter

nessiLahav · 2021-12-21T09:47:37Z

cucumber/authenticators_config/features/authn_config.feature

+    """
+    And authenticator "cucumber:webservice:conjur/authn-gcp" is disabled
+
+  Scenario: Authenticated user can not update authenticator without service-id


the headline is misleading , did you meant to test here an unauthorized user ?

yes fixed the scenario name

nessiLahav · 2021-12-21T09:49:08Z

cucumber/authenticators_config/features/authn_config.feature

+    """
+    enabled=true
+    """
+    Then the HTTP response status code is 401


if you will agree to my change above o thin that here you will get 404 instead of 401

we will get also 401 because the url is still valid

codeclimate · 2021-12-21T10:14:12Z

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]

+### Added
+- Added API endpoint to enable and disable GCP authenticator. [#2448](https://github.com/cyberark/conjur/pull/2448)


Lists should be surrounded by blank lines

codeclimate · 2021-12-21T11:01:47Z

Code Climate has analyzed commit e4f5fa5 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	1
Complexity	2

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 91.0% (0.0% change).

View more on Code Climate.

nessiLahav

LGTM

tzheleznyak

LGTM

alexkalish · 2022-02-09T21:13:12Z

Internal issue link.

aloncarmel111 requested a review from a team as a code owner December 20, 2021 08:20

aloncarmel111 requested review from nessiLahav, tzheleznyak and uCatu December 20, 2021 08:21

codeclimate bot reviewed Dec 20, 2021

View reviewed changes

app/domain/authentication/security/validate_webservice_is_authenticator.rb Show resolved Hide resolved

app/domain/authentication/security/validate_webservice_is_authenticator.rb Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

aloncarmel111 force-pushed the enable-gcp-authenticator branch from 1b2aaf0 to 00c271e Compare December 20, 2021 09:05

uCatu previously approved these changes Dec 20, 2021

View reviewed changes

tzheleznyak reviewed Dec 20, 2021

View reviewed changes

tzheleznyak previously approved these changes Dec 21, 2021

View reviewed changes

nessiLahav reviewed Dec 21, 2021

View reviewed changes

codeclimate bot reviewed Dec 21, 2021

View reviewed changes

aloncarmel111 added 3 commits December 21, 2021 12:31

Make service_id path parameter optional for authenticator whitelist

5a4427d

Add UT

b67cfa7

Add CHANGELOG

e4f5fa5

aloncarmel111 dismissed stale reviews from tzheleznyak and uCatu via e4f5fa5 December 21, 2021 10:36

aloncarmel111 force-pushed the enable-gcp-authenticator branch from 00c271e to e4f5fa5 Compare December 21, 2021 10:36

nessiLahav approved these changes Dec 21, 2021

View reviewed changes

tzheleznyak approved these changes Dec 21, 2021

View reviewed changes

aloncarmel111 merged commit 7313d48 into master Dec 21, 2021

aloncarmel111 deleted the enable-gcp-authenticator branch December 21, 2021 13:11

aloncarmel111 restored the enable-gcp-authenticator branch December 29, 2021 11:28

jtuttle mentioned this pull request Jan 19, 2022

Release v1.15.0+suite.1 cyberark/conjur-oss-suite-release#242

Merged

micahlee mentioned this pull request Jan 21, 2022

Enable GCP authenticator is not supported in authenticator API #2389

Closed

aloncarmel111 deleted the enable-gcp-authenticator branch April 13, 2022 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable gcp authenticator #2448

Enable gcp authenticator #2448

aloncarmel111 commented Dec 20, 2021

tzheleznyak Dec 20, 2021

tzheleznyak left a comment

nessiLahav Dec 21, 2021

aloncarmel111 Dec 21, 2021

nessiLahav Dec 21, 2021

aloncarmel111 Dec 21, 2021

nessiLahav Dec 21, 2021

aloncarmel111 Dec 21, 2021

codeclimate bot Dec 21, 2021

codeclimate bot commented Dec 21, 2021

nessiLahav left a comment

tzheleznyak left a comment

alexkalish commented Feb 9, 2022

Enable gcp authenticator #2448

Enable gcp authenticator #2448

Conversation

aloncarmel111 commented Dec 20, 2021

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

tzheleznyak Dec 20, 2021

Choose a reason for hiding this comment

tzheleznyak left a comment

Choose a reason for hiding this comment

nessiLahav Dec 21, 2021

Choose a reason for hiding this comment

aloncarmel111 Dec 21, 2021

Choose a reason for hiding this comment

nessiLahav Dec 21, 2021

Choose a reason for hiding this comment

aloncarmel111 Dec 21, 2021

Choose a reason for hiding this comment

nessiLahav Dec 21, 2021

Choose a reason for hiding this comment

aloncarmel111 Dec 21, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 21, 2021

Choose a reason for hiding this comment

codeclimate bot commented Dec 21, 2021

nessiLahav left a comment

Choose a reason for hiding this comment

tzheleznyak left a comment

Choose a reason for hiding this comment

alexkalish commented Feb 9, 2022