Skip to content
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

[WIP] Add eIDAS support #658

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Conversation

ioparaskev
Copy link
Contributor

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@codecov
Copy link

codecov bot commented Jan 6, 2020

Codecov Report

Merging #658 into master will increase coverage by 0.33%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #658      +/-   ##
==========================================
+ Coverage   65.05%   65.39%   +0.33%     
==========================================
  Files         102      104       +2     
  Lines       25670    25812     +142     
==========================================
+ Hits        16700    16879     +179     
+ Misses       8970     8933      -37     
Impacted Files Coverage Δ
src/saml2/extension/node_country.py 88.00% <88.00%> (ø)
src/saml2/utility/__init__.py 92.85% <92.85%> (ø)
src/saml2/config.py 85.33% <93.67%> (+1.23%) ⬆️
src/saml2/metadata.py 55.00% <95.45%> (+9.82%) ⬆️
src/saml2/version.py 100.00% <0.00%> (ø)
src/saml2/__init__.py 87.73% <0.00%> (+0.14%) ⬆️
src/saml2/client_base.py 75.73% <0.00%> (+1.71%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79f5fad...4c93775. Read the comment docs.

src/saml2/metadata.py Outdated Show resolved Hide resolved
@ioparaskev ioparaskev changed the title WIP Add eIDAS support [WIP] Add eIDAS support Jan 9, 2020
src/saml2/config.py Outdated Show resolved Hide resolved
@ioparaskev
Copy link
Contributor Author

test failures are due to a metadata file being valid until some hours ago. will open a different PR for that since it is unrelated to this one

- Adds schemas and auto-generated .py files for eIDAS NodeCountry and
NodeCountryType support
- Adds node_country as a recognized attribute in the configuration of an
  SP/IdP
- Adds node_country parsing for the construction of the entity
  descriptor as an extension element
- Adds configs and test classes for eIDAS support
- Adds validate method in Config class to be used for configuration
  validation checks
- Adds eIDASConfig as base eIDAS config class to host commonly (between
  IdP and SP) used validations and functions
- Adds eIDASSPConfig class and override validate method with endpoint
  and keydescriptor validations based on eidas v1.2 specs
- Adds utility->config module to host config helper classes and
  functions
- Adds new ConfigValidationError for config error signaling
- Adds RuleValidator class to be used for config elements validation
  rule crafting
- Adds should_warning and must_error functions for signaling warnings
  and errors related to element rules using RFC2119 wording
- Adds requirement iso3166 to lookup if a country code is valid (country
  exists and code is in ISO 3166-1 alpha-2 format)
- Adds validator method in eIDASConfig class (since both IdP and SP MUST
  have the NodeCountry element)
- Adds tests related to NodeCountry validation
- Adds support for setting the application identifier based on eIDAS
  Message Format v1.1-2 document
- Adds validator for the existence of the identifier and the correct
  formatting
- Adds tests for configuration validation related to the identifier and
  test for the existence as an entity attribute element
- Adds support for setting the protocol versions supported based on
  eIDAS Message Format v1.1-2 document
- Adds validator for the existence of the protocol version and issues a
  warning if it doesn't exist
- Adds tests for configuration validation and for the existence of
  protocol version as an entity attribute element
- Adds helper functions `make_type` and `make_list` to make parsing the
  protocol version easier regardless of it being a single element or a
list of elements
- Removes RuleValidator class and uses a dict data structure with key as
  error message and value as the check
- Extracts [endpoint element/protocol version/application
  identifier/node country] fetching to methods since they are part of
the eIDASConfig and will be overriden by the eIDASIdPConfig
- Adds validation for organization info (name or display name or url)
  and triggers a warning if none is present
- Adds support and technical contact person validation (both should
  exist and have an email address) and triggers a warning if not both
are present
- Adds `not_empty` function to do a shallow truthiness check (aka nested
  values like [{}] will be considered not empty)
- Adds test for the above scenarios
- Adds check that entityid is an https url based on eIDAS SAML Message
  Format v.1.2. There's no way of really validating that the url is
correct (there are many scenarios for false positives and negatives) so
we only check that the url scheme is https as the specs define
- Adds test for the above rule
- Adds validation check to verify that the eIDAS SP has set the
  authn_requests_signed to True
- Adds tests for the above scenario
- Extract `assert_validation_errror` test function for config validation
  tests
- Adds validation check for eIDAS SP to verify that sp_type has been set
  (MUST be set) and that it is set to a valid value (private/public) as
stated in eIDAS SAML Message Format v.1.2 spec
- Adds tests for the aforementioned checks
- Extracts common (for SP/IdP) warning and error validators
- Moves eIDASSPConfig validate method to eIDASConfig class
- Prints the errors that happened during validation (was only printing a
  message not the actual errors)
- Sets all validators to return True/False instead of values. This
  ensures consistency for the validators structure and also solves
issues were when there were errors/warnings printed out validators that
weren't returning True/False as values were also printed as errors in
the report (specifically the KeyDescriptor check and the application
identifier format check)
- Adds inherited validators for eIDAS IdP config. Specifically the
  following rules related to the metadata:
1. entityid MUST be HTTPs URL
2. SingleLogoutElementService SHOULD NOT be declared
3. ArtifactResolutionService SHOULD NOT be declared
4. ManageNameIDService SHOULD NOT be declared
5. KeyDescriptor MUST be declared
6. Organization with minimal info (name/display name/url) SHOULD be
   provided
7. Contact Person of contactType technical with an email address SHOULD
   be provided
8. Contact Person of contactType support with an email address SHOULD
   be provided
9. eIDAS protocol version implemented SHOULD be provided
10. eIDAS application identifier SHOULD be provided and MUST have a
    specific format
11. Node country information MUST be declared and MUST follow the ISO
    3166-1 alpha-2 format
All the above are specified in eIDAS SAML Message Format v.1.2 spec
document.
- Renames eidas tests config files because there was a clash in paths
  loaded and it resulted to other irrelevant tests crashing (because
they loaded the eidas conf files that had different entity ids)
- Adds error validation rule to require want_authn_requests_signed to be
  set to True as defined in the eIDAS SAML Message Format v.1.2 spec
document
- Adds tests for the above scenario
- Adds support for exposing IdP supported attributes in IDPSSODescriptor
  as Attribute elements. Support is added in the config file under
service->idp->provided_attributes. provided_attributes alread existed as
a valid option for idp config but was not used. Supported attributes
MUST be published as Attribute elements in the metadata of the eIDAS IdP
as stated in eIDAS SAML Message Format v.1.2 spec document
- Adds error validation rule in eIDASIdPConfig to ensure
  provided_attributes MUST be set
- Adds test to verify the provided_attributes are exposed as Attribute
  elements under IDPSSODescriptor and for the error validation rule
- Adds support for defining supported LoA for notified and non-notified
  eIDAS nodes
- Exposes LoA as Attribute element and supported LoA as AttributeValue
  elements to eIDAS IdP metadata
@ioparaskev
Copy link
Contributor Author

Why would we use a different option name than what this thing is actually about?

Well there's no defined name for this. The spec calls it supported attributes (as an expression) but I think that the term "provided attributes" is more precise since these are the attributes that the IdP provides. This and the fact that this config option already existed without any use made me use it. I can change it to supported_attributes and leave the provided_attributes unused like it was if you think this is better in terms of attribute meaning

- Adds validation checks for LoA configuration of an eIDAS IdP to verify
  that notified LoA URIs are not published as non-notified and that
notified LoA URIs are only the ones that are officially described in the
eIDAS SAML Message Format v.1.2 spec document
- Adds tests for the above scenarios
- Adds a config check that warns the user that they SHOULD set the
  `hide_assertion_consumer_service` to True since the eIDAS SAML Message
Format v.1.2 spec document states that during an
AssertionConsumerServiceURL attribute SHOULD NOT exist in an
AuthnRequest element
- Adds error validation rule to for `sign_response` configuration option
  to True as the authn responses MUST be signed according to eIDAS SAML
Message Format v.1.2 spec document
- Adds tests to test the aforementioned validation rule
- Adds error validation rule to for `encrypt_assertion` configuration
  option to True as the assertions MUST be encrypted according to eIDAS
SAML Message Format v.1.2 spec document
- Adds tests to test the aforementioned validation rule
src/saml2/config.py Outdated Show resolved Hide resolved
src/saml2/config.py Outdated Show resolved Hide resolved
src/saml2/config.py Outdated Show resolved Hide resolved
src/saml2/config.py Outdated Show resolved Hide resolved
- Adds error validation rule to enforce `allow_unsolicited=False` for an
  eIDAS SP config since the eIDAS SAML Message Format v.1.2 spec
document states that unsolicited responses MUST NOT be accepted
- Adds tests for the above validation rule
src/saml2/config.py Outdated Show resolved Hide resolved
- Extracts the eIDAS LoA prefix for notified eIDs as a variable and
  constructs a new variable to contain a list of all accepted LoA for
notified eIDs
- Fixes fallback LoA lookup when `non_notified` or `notified` are unset.
  Returned None, now returns an empty list
- Adds tests to test the aforementioned fixes
- Replaces supported_loa and uses the already existing
  assurance_certification for setting the supported LoA of an eIDAS IdP
- Refactor tests and validation checks to use this config key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants