Skip to content

Commit

Permalink
merge master changes
Browse files Browse the repository at this point in the history
  • Loading branch information
tianj7 committed Sep 1, 2023
2 parents 8ca4004 + ea885f0 commit c9d6b6a
Show file tree
Hide file tree
Showing 33 changed files with 892 additions and 312 deletions.
33 changes: 22 additions & 11 deletions .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
{
"name": "CloudantDetector"
},
{
"name": "DiscordBotTokenDetector"
},
{
"name": "GitHubTokenDetector"
},
Expand Down Expand Up @@ -72,10 +75,6 @@
{
"path": "detect_secrets.filters.allowlist.is_line_allowlisted"
},
{
"path": "detect_secrets.filters.common.is_baseline_file",
"filename": ".secrets.baseline"
},
{
"path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies",
"min_level": 2
Expand Down Expand Up @@ -106,12 +105,6 @@
},
{
"path": "detect_secrets.filters.heuristic.is_templated_secret"
},
{
"path": "detect_secrets.filters.regex.should_exclude_file",
"pattern": [
"poetry.lock"
]
}
],
"results": {
Expand All @@ -133,6 +126,15 @@
"line_number": 7
}
],
"docs/fence_multifactor_authentication_guide.md": [
{
"type": "Secret Keyword",
"filename": "docs/fence_multifactor_authentication_guide.md",
"hashed_secret": "0f674908b6342fcf2a9842d04699cb008d57d399",
"is_verified": false,
"line_number": 38
}
],
"fence/blueprints/storage_creds/google.py": [
{
"type": "Private Key",
Expand Down Expand Up @@ -315,6 +317,15 @@
"line_number": 49
}
],
"tests/login/test_idp_oauth2.py": [
{
"type": "Secret Keyword",
"filename": "tests/login/test_idp_oauth2.py",
"hashed_secret": "f3bbbd66a63d4bf1747940578ec3d0103530e21d",
"is_verified": false,
"line_number": 8
}
],
"tests/migrations/test_a04a70296688.py": [
{
"type": "Hex High Entropy String",
Expand Down Expand Up @@ -368,5 +379,5 @@
}
]
},
"generated_at": "2023-09-01T18:49:42Z"
"generated_at": "2023-09-01T19:02:21Z"
}
22 changes: 20 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,14 +1,32 @@
language: python

dist: jammy
python:
- "3.9"

sudo: false

cache: pip

services:
- postgresql

addons:
postgresql: "9.6"
postgresql: "13"
apt:
sources:
- sourceline: deb http://apt.postgresql.org/pub/repos/apt/ jammy-pgdg main
13
key_url: https://www.postgresql.org/media/keys/ACCC4CF8.asc
packages:
- postgresql-13

before_install:
# Copy custom configs from the repo because PG-13 isn't set up to run like
# it normally does on Travis out of the box.
# Source: https://github.com/NCI-GDC/psqlgraph/blob/94f315db2c039217752cba85d9c63988f2059317/.travis.yml
- sudo cp travis/postgresql.conf /etc/postgresql/13/main/postgresql.conf
- sudo cp travis/pg_hba.conf /etc/postgresql/13/main/pg_hba.conf
- sudo pg_ctlcluster 13 main restart

install:
- pip install --upgrade pip
Expand Down
64 changes: 64 additions & 0 deletions docs/fence_multifactor_authentication_guide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# Fence Multifactor Authentication Guide

Fence is capable of using token claims from IdPs to identify when multifactor authentication (MFA) was used during the authentication process.

## File Level Enforcement
To restrict access to files to user who've authenticated with MFA, the following resource *MUST* be present in the indexd record's `authz`:
`/multifactor_auth`

And the following configs must be updated:
- fence-config.yaml
- user.yaml

### fence-config.yaml changes

MFA claim checking is configured on a per-IdP basis. For a given IdP, define the name of the claim in the id_token and is possible values that indicate MFA. If the id_token claim value matches at least one value in the configured multifactor_auth_claim_info.values, then "/multifactor_auth" resource will be assigned to the user.

For example, Okta may issue the following id_token when MFA is used:
```
{
"amr": ["otp", "pwd"],
"aud": "6joRGIzNCaJfdCPzRjlh",
"auth_time": 1311280970,
"exp": 1311280970,
"iat": 1311280970,
"idp": "00ok1u7AsAkrwdZL3z0g3",
"iss": "https://$"
"jti": "Tlenfse93dgkaksginv",
"sub": "00uk1u7AsAk6dZL3z0g3",
"ver": 1
}
```

And fence-config.yaml is configured as follows:
```
OPENID_CONNECT:
okta:
client_id: 'redacted'
client_secret: 'redacted'
multifactor_auth_claim_info:
claim: 'amr'
values: [ "mfa", "otp", "sms" ]
```

Then fence will assign the "/multifactor_auth" resource to the user in Arborist.

### user.yaml changes
The `mfa_policy` policy and `multifactor_auth` resource must be added to user.yaml so the appropriate policy and resource are created in arborist when usersync runs.

NOTE: The role_ids provided here are an example and should be changed to the appropriate arborist roles for the commons.

Add the following to the `resources` section:
```yaml
- name: multifactor_auth
```
Add the following the `policies` section:
```yaml
- id: mfa_policy
role_ids:
- read-storage
- read
resource_paths:
- /multifactor_auth
```
2 changes: 1 addition & 1 deletion fence/blueprints/link.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ def get(self):
code = flask.request.args.get("code")

if not config.get("MOCK_GOOGLE_AUTH", False):
google_response = flask.current_app.google_client.get_user_id(code)
google_response = flask.current_app.google_client.get_auth_info(code)
email = google_response.get("email")
else:
# if we're mocking google auth, mock response to include the email
Expand Down
26 changes: 25 additions & 1 deletion fence/blueprints/login/base.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import flask
from cdislogging import get_logger
from flask_restful import Resource
from urllib.parse import urlparse, urlencode, parse_qsl

Expand All @@ -7,6 +8,8 @@
from fence.config import config
from fence.errors import UserError

logger = get_logger(__name__)


class DefaultOAuth2Login(Resource):
def __init__(self, idp_name, client):
Expand Down Expand Up @@ -63,6 +66,7 @@ def __init__(
username_field="email",
email_field="email",
id_from_idp_field="sub",
app=flask.current_app,
):
"""
Construct a resource for a login callback endpoint
Expand All @@ -84,6 +88,10 @@ def __init__(
self.username_field = username_field
self.email_field = email_field
self.id_from_idp_field = id_from_idp_field
self.is_mfa_enabled = "multifactor_auth_claim_info" in config[
"OPENID_CONNECT"
].get(self.idp_name, {})
self.app = app

def get(self):
# Check if user granted access
Expand All @@ -109,7 +117,7 @@ def get(self):
return flask.redirect(location=final_redirect_url)

code = flask.request.args.get("code")
result = self.client.get_user_id(code)
result = self.client.get_auth_info(code)
username = result.get(self.username_field)
if not username:
raise UserError(
Expand All @@ -125,6 +133,22 @@ def get(self):

def post_login(self, user=None, token_result=None, **kwargs):
prepare_login_log(self.idp_name)
if token_result:
username = token_result.get(self.username_field)
if self.is_mfa_enabled:
if token_result.get("mfa"):
logger.info(f"Adding mfa_policy for {username}")
self.app.arborist.grant_user_policy(
username=username,
policy_id="mfa_policy",
)
return
else:
logger.info(f"Revoking mfa_policy for {username}")
self.app.arborist.revoke_user_policy(
username=username,
policy_id="mfa_policy",
)


def prepare_login_log(idp_name):
Expand Down
2 changes: 1 addition & 1 deletion fence/blueprints/login/ras.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,4 @@ def post_login(self, user=None, token_result=None, id_from_idp=None):
user=user, refresh_token=refresh_token, expires=expires + issued_time
)

super(RASCallback, self).post_login(id_from_idp=id_from_idp)
super(RASCallback, self).post_login(token_result=token_result)
34 changes: 34 additions & 0 deletions fence/config-default.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,9 @@ OPENID_CONNECT:
user_id_field: '' # optional (default "sub"); claims field to get the user_id from
email_field: '' # optional (default "email"); claims field to get the user email from
scope: '' # optional (default "openid")
multifactor_auth_claim_info: # optional, include if you're using arborist to enforce mfa on a per-file level
claim: '' # claims field that indicates mfa, either the acr or acm claim.
values: [ "" ] # possible values that indicate mfa was used. At least one value configured here is required to be in the token
# These Google values must be obtained from Google's Cloud Console
# Follow: https://developers.google.com/identity/protocols/OpenIDConnect
#
Expand Down Expand Up @@ -181,6 +184,9 @@ OPENID_CONNECT:
client_secret: ''
redirect_url: '{{BASE_URL}}/login/ras/callback'
scope: 'openid email profile ga4gh_passport_v1'
# multifactor_auth_claim_info:
# claim: 'acr'
# values: [ 'https://stsstg.nih.gov/assurance/aal/2' ]
# if mock is true, will fake a successful login response for login
# WARNING: DO NOT ENABLE IN PRODUCTION (for testing purposes only)
mock: false
Expand All @@ -207,6 +213,9 @@ OPENID_CONNECT:
# WARNING: DO NOT ENABLE IN PRODUCTION (for testing purposes only)
mock: false
mock_default_user: '[email protected]'
# multifactor_auth_claim_info:
# claim: 'amr'
# values: [ "mfa", "otp", "rsa", "ngcmfa", "wiaormfa" ]
# For information on configuring an Okta tenant as an OIDC IdP refer to Okta documentation at:
# https://developer.okta.com/docs/reference/api/oidc/#2-okta-as-the-identity-platform-for-your-app-or-api
okta:
Expand All @@ -215,6 +224,9 @@ OPENID_CONNECT:
client_secret: ''
redirect_url: '{{BASE_URL}}/login/okta/login/'
scope: 'openid email'
# multifactor_auth_claim_info:
# claim: 'amr'
# values: [ "mfa", "otp", "sms" ]
cognito:
# You must create a user pool in order to have a discovery url
discovery_url: 'https://cognito-idp.{REGION}.amazonaws.com/{USER-POOL-ID}/.well-known/openid-configuration'
Expand All @@ -241,6 +253,9 @@ OPENID_CONNECT:
# WARNING: DO NOT ENABLE IN PRODUCTION (for testing purposes only)
mock: false
mock_default_user: 'http://cilogon.org/serverT/users/64703'
# multifactor_auth_claim_info:
# claim: 'acr'
# values: [ "https://refeds.org/profile/mfa" ]
synapse:
discovery_url: ''
client_id: ''
Expand Down Expand Up @@ -532,6 +547,25 @@ dbGaP:
#
# NOTE: when this is "false" the above would become "phs000123"
parse_consent_code: true
# When a dbGaP study authorizes access to child studies through a parent study ID,
# you can use this mapping. When a user gets access to the first ID, they automatically
# get access to the list of projects to the right.
#
# There's usually a note in the "Authorized Access" section of the dbGaP study page
# (https://www.ncbi.nlm.nih.gov/projects/gap/cgi-bin/study.cgi?study_id=phs001843.v1.p2)
# along the lines of:
# Note: The data for this study is collected as a substudy of
# phs001194.v3.p2. dbGaP Authorized Access requests for
# this data should be made for study phs001194.v3.p2 and
# not phs001843.v1.p2
#
# There are also other dbGaP APIs that expose this parent/child mapping.
# Example: https://dbgap.ncbi.nlm.nih.gov/ss/dbgapssws.cgi?request=Study&phs=000571&v=6
#
# If `parse_consent_code` is true, then a user will be given access to the exact
# same consent codes in the child studies
parent_to_child_studies_mapping:
# 'phs001194': ['phs000571', 'phs001843']
# A consent of "c999" can indicate access to that study's "exchange area data"
# and when a user has access to one study's exchange area data, they
# have access to the parent study's "common exchange area data" that is not study
Expand Down
33 changes: 31 additions & 2 deletions fence/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,18 +130,47 @@ def post_process(self):
"BILLING_PROJECT_FOR_SA_CREDS or BILLING_PROJECT_FOR_SIGNED_URLS is set to a non-None value. "
"SESSION_ALLOWED_SCOPES includes `google_credentials`. Removing "
"`google_credentials` from USER_ALLOWED_SCOPES as this could allow "
"end-users to indescriminently bill our default project. Clients are inheritently "
"end-users to indiscriminately bill our default project. Clients are inherently "
"trusted, so we do not restrict this scope for clients."
)
self._configs["SESSION_ALLOWED_SCOPES"].remove("google_credentials")

if (
not self._configs["ENABLE_VISA_UPDATE_CRON"]
and self._configs["GLOBAL_PARSE_VISAS_ON_LOGIN"] != False
and self._configs["GLOBAL_PARSE_VISAS_ON_LOGIN"] is not False
):
raise Exception(
"Visa parsing on login is enabled but `ENABLE_VISA_UPDATE_CRON` is disabled!"
)

for idp_id, idp in self._configs.get("OPENID_CONNECT", {}).items():
mfa_info = idp.get("multifactor_auth_claim_info")
if mfa_info and mfa_info["claim"] not in ["amr", "acr"]:
logger.warning(
f"IdP '{idp_id}' is using multifactor_auth_claim_info '{mfa_info['claim']}', which is neither AMR or ACR. Unable to determine if a user used MFA. Fence will continue and assume they have not used MFA."
)

self._validate_parent_child_studies(self._configs["dbGaP"])

@staticmethod
def _validate_parent_child_studies(dbgap_configs):
if isinstance(dbgap_configs, list):
configs = dbgap_configs
else:
configs = [dbgap_configs]

all_parent_studies = set()
for dbgap_config in configs:
parent_studies = dbgap_config.get(
"parent_to_child_studies_mapping", {}
).keys()
conflicts = parent_studies & all_parent_studies
if len(conflicts) > 0:
raise Exception(
f"{conflicts} are duplicate parent study ids found in parent_to_child_studies_mapping for "
f"multiple dbGaP configurations."
)
all_parent_studies.update(parent_studies)


config = FenceConfig(DEFAULT_CFG_PATH)
2 changes: 1 addition & 1 deletion fence/resources/openid/cilogon_oauth2.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def get_auth_url(self):

return uri

def get_user_id(self, code):
def get_auth_info(self, code):
try:
token_endpoint = self.get_value_from_discovery_doc(
"token_endpoint", "https://cilogon.org/oauth2/token"
Expand Down
2 changes: 1 addition & 1 deletion fence/resources/openid/cognito_oauth2.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def get_auth_url(self):

return uri

def get_user_id(self, code):
def get_auth_info(self, code):
"""
Exchange code for tokens, get email from id token claims.
Return dict with "email" field on success OR "error" field on error.
Expand Down
Loading

0 comments on commit c9d6b6a

Please sign in to comment.