Integrate 11.7 release branch (#1884)

* Only submit code coverage report when Code Climate is successfully prepared * Merge in GHSA-c7x2-6g4j-327p changes (API Key Timing) Signed-off-by: Andy Tinkham <[email protected]> * Merge in GHSA-qhjf-g9gm-64jq (Self api-key rotation) Signed-off-by: Andy Tinkham <[email protected]> * Release v1.9.0 Co-authored-by: Andy Tinkham <[email protected]>
cyberark · Oct 16, 2020 · 257237a · 257237a
1 parent f1bf3b6
commit 257237a
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 41 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,26 +6,36 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 ### Added
+- [Documentation](./UPGRADING.md) explaining how to upgrade a Conjur server deployed in a
+  Docker Compose environment. [cyberark/conjur#1528](https://github.com/cyberark/conjur/issues/1528), [cyberark/conjur#1584](https://github.com/cyberark/conjur/issues/1584)
+- When Conjur starts, we now convert blank environment variables to nil. This ensures we treat empty environment values as 
+  if the environment variable is not present, rather than attempting to use the empty string value. [cyberark/conjur#1841](https://github.com/cyberark/conjur/issues/1841)
+
+### Changed
+- The "inject_client_cert" request now returns 202 Accepted instead of 200 OK to
+  indicate that the cert injection has started but not necessarily completed.
+  [cyberark/conjur#1848](https://github.com/cyberark/conjur/issues/1848)
+
+### Fixed
+- Conjur now verifies that Kubernetes Authenticator variables exist and have value before retrieving them so that a 
+  proper error will be raised if they aren't.
+  [cyberark/conjur#1315](https://github.com/cyberark/conjur/issues/1315)
+
+## [1.9.0] - 2020-08-31
+### Added
 - Hosts can authenticate from Google Compute Engines (GCE) using a GCE instance
   identity token. See [design](design/authenticators/authn_gcp/authn_gcp_solution_design.md)
   for details ([cyberark/conjur#1711](https://github.com/cyberark/conjur/issues/1711)).
-- Add `/whoami` API endpoint for improved supportability and debugging for access
+- New `/whoami` API endpoint for improved supportability and debugging for access
   tokens and client IP address determination. [cyberark/conjur#1697](https://github.com/cyberark/conjur/issues/1697)
 - `TRUSTED_PROXIES` is validated at Conjur startup to ensure that it contains
   valid IP addresses and/or address ranges in CIDR notation.
   [cyberark/conjur#1727](https://github.com/cyberark/conjur/issues/1727)
 - The `/authenticate` endpoint now returns a text/plain base64 encoded access token
   if the `Accept-Encoding` request header includes `base64`.
   [cyberark/conjur#151](https://github.com/cyberark/conjur/issues/151)
-- [Documentation](./UPGRADING.md) explaining how to upgrade a Conjur server deployed in a
-  Docker Compose environment. [cyberark/conjur#1528](https://github.com/cyberark/conjur/issues/1528), [cyberark/conjur#1584](https://github.com/cyberark/conjur/issues/1584)
-- When Conjur starts, we now convert blank environment variables to nil. This ensures we treat empty environment values as 
-  if the environment variable is not present, rather than attempting to use the empty string value. [cyberark/conjur#1841](https://github.com/cyberark/conjur/issues/1841)
 
 ### Changed
-- The "inject_client_cert" request now returns 202 Accepted instead of 200 OK to
-  indicate that the cert injection has started but not necessarily completed.
-  [cyberark/conjur#1848](https://github.com/cyberark/conjur/issues/1848)
 - The Conjur server request logs now records the same IP address used by audit
   logs and network authentication filters with the `restricted_to` attribute.
   [cyberark/conjur#1719](https://github.com/cyberark/conjur/issues/1719)
@@ -44,9 +54,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - A new database migration step updates the fingerprints in slosilo. The FIPS compliance
   update in `v1.8.0` caused the previous fingerprints to be invalid.
   [cyberark/conjur#1584](https://github.com/cyberark/conjur/issues/1584)
-- Conjur now verifies that Kubernetes Authenticator variables exist and have value before retrieving them so that a 
-  proper error will be raised if they aren't.
-  [cyberark/conjur#1315](https://github.com/cyberark/conjur/issues/1315)
+
+### Security
+- Replaces string comparison with Secure Compare to prevent timing attacks against
+  the API authentication endpoint. [Security Bulletin](https://github.com/cyberark/conjur/security/advisories/GHSA-c7x2-6g4j-327p)
+- Roles can no longer rotate their own API key using only an access token.
+  [Security Bulletin](https://github.com/cyberark/conjur/security/advisories/GHSA-qhjf-g9gm-64jq)
 
 ## [1.8.1] - 2020-07-14
 ### Fixed
@@ -430,7 +443,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Added
 - The first tagged version.
 
-[Unreleased]: https://github.com/cyberark/conjur/compare/v1.8.1...HEAD
+[Unreleased]: https://github.com/cyberark/conjur/compare/v1.9.0...HEAD
+[1.9.0]: https://github.com/cyberark/conjur/compare/v1.8.1...v1.9.0
 [1.8.1]: https://github.com/cyberark/conjur/compare/v1.7.0...v1.8.1
 [1.8.0]: https://github.com/cyberark/conjur/compare/v1.7.4...v1.8.0
 [1.7.4]: https://github.com/cyberark/conjur/compare/v1.7.3...v1.7.4

diff --git a/Jenkinsfile b/Jenkinsfile
@@ -68,9 +68,12 @@ pipeline {
 
     stage('Prepare For CodeClimate Coverage Report Submission'){
       steps {
-        script {
-          ccCoverage.dockerPrep()
-          sh 'mkdir -p coverage'
+        catchError(buildResult: 'SUCCESS', stageResult: 'FAILURE') {
+          script {
+            ccCoverage.dockerPrep()
+            sh 'mkdir -p coverage'
+            env.CODE_CLIMATE_PREPARED = "true"
+          }
         }
       }
     }
@@ -233,6 +236,11 @@ pipeline {
     }
 
     stage('Submit Coverage Report'){
+      when {
+        expression {
+          env.CODE_CLIMATE_PREPARED == "true"
+        }
+      }
       steps{
         sh 'ci/submit-coverage'
       }

diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-1.8.1
+1.9.0
diff --git a/app/controllers/credentials_controller.rb b/app/controllers/credentials_controller.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+# This class smells of :reek:RepeatedConditional for the multiple calls
+# to `authentication.self?`
 class CredentialsController < ApplicationController
   include BasicAuthenticator
   include TokenUser
@@ -12,23 +14,23 @@ class CredentialsController < ApplicationController
   # The username can also come from an +id+ parameter, if the operation will be performed on a different
   # user than the authenticated user.
   before_action :accept_id_parameter
-    
+
   # Ensure that the referenced role exists.
   before_action :find_role
 
   # Token authentication cannot be used to update +self+ credentials.
   before_action :restrict_token_auth
-  
+
   # Users can update their own record, and +update+ privilege on the authn service enables a superuser
   # to update any user's record.
   before_action :authorize_self_or_update, only: [ :rotate_api_key ]
-    
+
   # Users are always permitted to perform some operations on their own record.
   before_action :authorize_self, except: [ :rotate_api_key ]
 
   # Ensure the credentials exist if they will be accessed or modified.
   before_action :ensure_credentials, only: [ :update_password, :rotate_api_key, :login ]
-    
+
   # Update the authenticated user's password. The implication of this is that if you can login as a user, you can change
   # that user's password.
   #
@@ -41,10 +43,10 @@ def update_password
       password: password,
       client_ip: request.ip
     )
-        
+
     head 204
   end
-  
+
   # Rotate a user API key.
   #
   # The new API key is in the request body.
@@ -56,9 +58,9 @@ def rotate_api_key
     )
     render plain: @role.credentials.api_key
   end
-  
+
   protected
-  
+
   def authenticate_client
     authentication.authenticated_role = Role[token_user.roleid] if token_user?
     perform_basic_authn
@@ -78,30 +80,30 @@ def accept_id_parameter
     authentication.selected_role = Role[Role.make_full_id(params[:role], account)] if params[:role]
     true
   end
-      
+
   def find_role
     raise Unauthorized, "Role not found" unless @role = authentication.apply_to_role
   end
-  
+
   # Ensure that the current role has credentials.
   def ensure_credentials
     @role.credentials ||= Credentials.new(role: @role)
   end
-  
+
   # Don't permit token auth when manipulating 'self' record.
   def restrict_token_auth
-    if authentication.selected_role
-      true
-    else
+    if authentication.self?
       raise Unauthorized, "Credential strength is insufficient" unless authentication.basic_user?
+    else
+      true
     end
   end
 
   # The authenticated user represents a user in this account.
   def authorize_self
     raise Unauthorized, "Operation attempted against foreign user" unless authentication.self?
   end
-  
+
   # The operation is allowed on the authenticated user's own record, or on the record
   # indicated by +id+ if the authenticated user has +update+ privilege on the authn service.
   def authorize_self_or_update
@@ -111,7 +113,7 @@ def authorize_self_or_update
     raise Unauthorized, "Insufficient privilege" unless resource = @role.resource
     raise Unauthorized, "Insufficient privilege" unless authentication.authenticated_role.allowed_to? "update", resource
   end
-  
+
   # Read privilege is always granted.
   def authorize_self_or_read
     true

diff --git a/app/models/credentials.rb b/app/models/credentials.rb
@@ -41,7 +41,7 @@ def random_api_key
   def as_json
     { }
   end
-  
+
   def restricted_to
     self[:restricted_to].map { |cidr| Util::CIDR.new(cidr) }
   end
@@ -57,6 +57,9 @@ def authenticate pwd
 
   def valid_password? pwd
     bc = BCrypt::Password.new self.encrypted_hash
+    # This `==` is implemented by BCrypt' Password class (link:
+    #     https://www.rubydoc.info/github/codahale/bcrypt-ruby/BCrypt/Password#==-instance_method)
+    # The comparison occurs against two BCrypt hashes, thus, is not a timing attack concern
     if bc == pwd
       self.update password: pwd if bc.cost != BCRYPT_COST
       return true
@@ -69,32 +72,32 @@ def valid_password? pwd
 
   def valid_api_key? key
     return false if expired?
-    key && (key == api_key) 
+    key && ActiveSupport::SecurityUtils.secure_compare(key, api_key)
   end
-  
+
   def validate
     super
 
     validates_presence [ :api_key ]
 
     errors.add(:password, ::Errors::Conjur::InsufficientPasswordComplexity.new.to_s) if @plain_password && (@plain_password.index("\n") || @plain_password !~ VALID_PASSWORD_REGEX)
   end
-  
+
   def before_validation
     super
-    
+
     self.api_key ||= self.class.random_api_key
   end
-  
+
   def rotate_api_key
     self.api_key = self.class.random_api_key
   end
-  
+
   private
-  
+
   def expired?
     return false unless self.expiration
-    
+
     self.expiration <= Time.now
   end
 end
diff --git a/cucumber/api/features/rotate_api_key.feature b/cucumber/api/features/rotate_api_key.feature
@@ -14,6 +14,21 @@ Feature: Rotate the API key of a role
     Then the result is the API key for user "alice"
     And the HTTP response content type is "text/plain"
 
+  Scenario: API key can be used to rotate API key
+    When I can PUT "/authn/cucumber/api_key" with username "alice" and password ":cucumber:user:alice_api_key"
+    Then the result is the API key for user "alice"
+    And the HTTP response content type is "text/plain"
+
+  Scenario: Access token can not be used to rotate API key
+    Given I login as "alice"
+    When I PUT "/authn/cucumber/api_key"
+    Then the HTTP response status code is 401
+
+  Scenario: Access token can not be used to rotate API key using role parameter with self role value
+    Given I login as "alice"
+    When I PUT "/authn/cucumber/api_key?role=user:alice"
+    Then the HTTP response status code is 401
+
   @logged-in
   Scenario: The API key cannot be rotated by foreign role without 'update' privilege
     Given I create a new admin-owned user "bob"