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

Connection Security Allowed Values #961

Open
14 tasks
brian-ruf opened this issue Dec 4, 2024 · 25 comments · Fixed by #1021
Open
14 tasks

Connection Security Allowed Values #961

brian-ruf opened this issue Dec 4, 2024 · 25 comments · Fixed by #1021

Comments

@brian-ruf
Copy link
Contributor

brian-ruf commented Dec 4, 2024

Constraint Task

In order to increase consistency in the responses to the "connection-security" property in components, we need an allowed values list that allows additional entries in case there are additional choices beyond those we considered.

Intended Outcome

  • Provide suggested allowed values for "connection-security" property/extension.

Syntax Type

This is required core OSCAL syntax.

Allowed Values

Allowed values are being discussed in the comments below starting at this comment.

Metapath(s) to Content

TARGET: //component/prop[@name='connection-security' and @ns='http://fedramp.gov/ns/oscal']/@value

Purpose of the OSCAL Content

To provide additional information about the security of the connection as part of risk management trade-off decisions.

Dependencies

None

Acceptance Criteria

  • All OSCAL adoption content affected by the change in this issue have been updated in accordance with the Documentation Standards.
    • Explanation is present and accurate
    • sample content is present and accurate
    • Metapath is present, accurate, and does not throw a syntax exception using oscal-cli metaschema metapath eval -e "expression".
  • All constraints associated with the review task have been created
  • The appropriate example OSCAL file is updated with content that demonstrates the FedRAMP-compliant OSCAL presentation.
  • The constraint conforms to the FedRAMP Constraint Style Guide.
    • All automated and manual review items that identify non-conformance are addressed; or technical leads (David Waltermire; AJ Stein) have approved the PR and “override” the style guide requirement.
  • Known good test content is created for unit testing.
  • Known bad test content is created for unit testing.
  • Unit testing is configured to run both known good and known bad test content examples.
  • Passing and failing unit tests, and corresponding test vectors in the form of known valid and invalid OSCAL test files, are created or updated for each constraint.
  • A Pull Request (PR) is submitted that fully addresses the goals section of the User Story in the issue.
  • This issue is referenced in the PR.

Other information

None.

@aj-stein-gsa
Copy link
Contributor

I looked around old docs and the extensions when coding the other issue, where did these values come from outside of OSCAL, @brian-ruf?

@aj-stein-gsa
Copy link
Contributor

Also SFTP protocol happens inside of SSH tunnels so maybe it is nice to have but isn't it redundant if we are talking about connection security and not the application layer protocol?

@brian-ruf
Copy link
Contributor Author

These are new and was a quick list based on light research. It likely does need vetting and modification.

@brian-ruf
Copy link
Contributor Author

brian-ruf commented Dec 6, 2024

Moving the draft list from the original issue to this comment as it is only an early, suggested draft and needs to be reviewed/shaped/vetted.

Allow Others: YES

Allowed values:

  • tls-1.0: Transport Layer Security (TLS) Version 1.0
  • tls-1.1: Transport Layer Security (TLS) Version 1.1
  • tls-1.2: Transport Layer Security (TLS) Version 1.2
  • tls-1.3: Transport Layer Security (TLS) Version 1.3
  • ssl-1.0: Secure Sockets Layer (SSL) 1.0
  • ssl-2.0: Secure Sockets Layer (SSL) 2.0
  • ssl-3.0: Secure Sockets Layer (SSL) 3.0
  • ipsec: Internet Protocol Security (IPSec)
  • ipsec-ikev1: Internet Protocol Security (IPSec) Internet Key Exchange (IKE) Version 1
  • ipsec-ikev2: Internet Protocol Security (IPSec) Internet Key Exchange (IKE) Version 2
  • ssh-1: Secure Shell 1 (SSH-1)
  • ssh-2: Secure Shell 2 (SSH-2)
  • vpn: Virtual Private Network (VPN)

@brian-ruf
Copy link
Contributor Author

brian-ruf commented Dec 6, 2024

Outside GitHub one concern raised was whether we should allow values where FedRAMP reviewers would not accept. The concern was that we don't want users believing any of the defined allowed values are acceptable to FedRAMP.

We need to balance this against the idea that when an SSP is delivered to an assessor, AO, or the FedRAMP PMO it should reflect the current state of the system - even if some aspects are not normally acceptable. In other words, if we eliminate choices that FedRAMP normally finds unacceptable, we block the ability to document that unacceptable choice if it is in-fact the current state of the system.

Further, there are instances - especially with LI-SaaS - where an AO is willing to accept a less secure connection because the information being passed isn't sensitive or because the external system that only supports insecure connection types is the only source of required information.

One way to balance these competing concerns is to have a larger list of allowed values, but also a constraint that raises a warning if the selected value is not one of the choices commonly acceptable to FedRAMP Reviewers and AOs. We could also annotate the text for certain allowed values as "preferred", "depreciated" or "insecure".

@aj-stein-gsa
Copy link
Contributor

Outside GitHub one concern raised was whether we should allow values where FedRAMP reviewers would not accept. The concern was that we don't want users believing any of the defined allowed values are acceptable to FedRAMP.

We need to balance this against the idea that when an SSP is delivered to an assessor, AO, or the FedRAMP PMO it should reflect the current state of the system - even if some aspects are not normally acceptable. In other words, if we eliminate choices that FedRAMP normally finds unacceptable, we block the ability to document that unacceptable choice if it is in-fact the current state of the system.

I actually completely agree with this perspective. Maybe we should warn based on learned experience, but we have to allow those values as the end of the day.

I didn't want to nitpick, if anything I was unclear why SFTP was a separation connection security ... do we say protocol? ... from SSH. I understand that is probably a common way of phrasing it and FedRAMP may receive packages with that notion, and VPN is general, but I assume the reflects where everyone is today, secure or not.

I am happy with the above requirements to move forward as-is given your comments and the discussion. 🫡

@brian-ruf
Copy link
Contributor Author

@aj-stein-gsa I just removed the SFTP choices. I had done some quick searches and it was in a list that also included SSH. I hadn't considered that SFTP tunnels through SSH by design. The only portion of this list where I am aware of clear FedRAMP guidance is the depreciation of SSH and TLS 1.1 with a requirement for TLS 1.2 or 1.3. Even that is old information and I suspect 1.2 is at least depreciated if not outright unacceptable.

This is another area where we should really ask the review team:

  • what they might possibly see; and
  • what they currently accept

@DimitriZhurkin DimitriZhurkin self-assigned this Dec 18, 2024
@DimitriZhurkin
Copy link

@brian-ruf,

A couple of observations, if I may:

  1. We haven't updated https to http globally yet. Hence, for the time being, I'll keep ns='https://fedramp.gov/ns/oscal'.
  2. Setting allow-other="yes" will never fail.
  3. Is it a warning?

@brian-ruf
Copy link
Contributor Author

Correct - having allow-others='yes' does prevent us from raising an error. This is not an exhaustive list, and indeed there was some interest in massaging this list before allowed-values are created.

In any case, we can and should warn if the provided value is not recognized in the allowed-values list. The biggest reason for having allowed-values here is to introduce consistency in how the data is presented for the choices we do know about.

We don't want to deal with tls-1.2, tls1.2, tls_1.2, tls.1.2 and tls12 (and all the same again with capital letters).

@DimitriZhurkin
Copy link

Well, the problem is allow-other="yes".

Since I cannot create a failing test, validations fail, and I won't be able to merge the branch into GitHub.

To solve the problem, here's what I did:

  1. In fedramp-external-allowed-values.xml, created allowed values, but set allow-other="no".
  2. In fedramp-external-constraints.xml, created a WARNING constraint with the following message:
    Component {@uuid} ({path(.)}) contains a 'connection-security' property with a non-FedRAMP value. While you may specify the non-FedRAMP value, you SHOULD make sure that the value is valid.

Let me know if such setup is acceptable.

@brian-ruf
Copy link
Contributor Author

Are you saying it's impossible to have a list of acceptable values with allow-others=yes in our CI/CD pipeline?

That sounds like a problem with the pipeline.

I don't think this is acceptable in that other tools interpreting the metaschema are being incorrectly instructed not to allow other values.

It sounds like the issue is that the CI/CD pipeline needs to be adjusted to allow no failure test when allow-others="yes"

@DimitriZhurkin
Copy link

It's been like that for quite a while. I've already faced that dilemma a couple of months ago.

@aj-stein-gsa
Copy link
Contributor

aj-stein-gsa commented Dec 18, 2024

It sounds like the issue is that the CI/CD pipeline needs to be adjusted to allow no failure test when allow-others="yes"

In this sense, you are saying "I accept anything, but I also want an error for it using the same allowed-values construct and no alternative." Is we want to recommend something level="WARNING", we need to use expect for that. This is not our CI/CD pipeline, this is a fundamental concept in the Metaschema specification. (I am not saying that I agree with it, but I had repeated my misunderstanding of this concept freque

What is the specific use-case we want here? We can do it redundantly with an expect. The alternative is to have an @extensible values set and coordinate with NIST upstream (if it is defined upstream, if not we can. We can definitely discuss if necessary.

@brian-ruf
Copy link
Contributor Author

brian-ruf commented Dec 19, 2024

I'm not saying "I accept anything, but also want an error".

The opposite. I am saying I want a suggested set of allowed values, and want to accept other values that we may not have reasonably predicted.

I expect OSCAL tools to use the allowed-values construct to offer these pre-defined choices to the user. If the user selects one of the suggested choices, I expect the tool to use our pre-defined value associated with that choice. But I also need to allow that tool to handle other choices that we failed to reasonably predict. The "allow-others" flag is how the tool knows whether it should allow the user to provide other choices or strictly limit the user to the defined choices.

I feel like I am being told that we cannot define allowed values unless they are used to produce a warning/error. And that allow-others=yes is never valid.

If true, why is the allow-others choice available at all?
Under what circumstances would allow-others=yes be valid?

I am fine with a warning if none of the allowed values are present, but I wasn't asking for that.

I am not comfortable with explicitly defining/stating in our metaschema that no other values are allowed when that is not true.
Sorry if I'm missing something. I'll ultimately yield to your decision.

@aj-stein-gsa
Copy link
Contributor

aj-stein-gsa commented Dec 19, 2024

I'm not saying "I accept anything, but also want an error".

Was trying to frame the language in a personified way and that was clearly not helpful. I was posing that to say from the Metaschema implementation and saying why it is not coded to handle it that way.

The opposite. I am saying I want a suggested set of allowed values, and want to accept other values that we may not have reasonably predicted.

Understood, I am just saying I thought this was actionable in the past from the Metaschema processing and specification was enhanced at one point for allowed-values because I brought forward this confusion IIRC, that is why I linked to it. I have made this mistake early in my time here twice as two how that can play out with error messaging (not that you cannot define them, see below).

I expect OSCAL tools to use the allowed-values construct to offer these pre-defined choices to the user. If the user selects one of the suggested choices, I expect the tool to use our pre-defined value associated with that choice. But I also need to allow that tool to handle other choices that we failed to reasonably predict. The "allow-others" flag is how the tool knows whether it should allow the user to provide other choices or strictly limit the user to the defined choices.

I feel like I am being told that we cannot define allowed values unless they are used to produce a warning/error. And that allow-others=yes is never valid.

I am not comfortable with explicitly defining/stating in our metaschema that no other values are allowed when that is not true. Sorry if I'm missing something. I'll ultimately yield to your decision.

I am fine with defining them. What Dimitri is saying, and I am trying to reiterate (and I did so too abruptly without further explanation) is that testing recommended values through negative testing (putting a wrong value) with allowed-values/@allow-other="yes"] is not practical for constraint validation. And you're it's only for that. We can add them, just good luck testing them. That was my only point. And maybe that is ok. Iff we want a warning for not using those recommended values, we have to come up with a solution that doesn't depend on allowed-values if you want a message in error messages, but to feed drop-down messaging it is good to call this out as another use case explicitly.

@DimitriZhurkin
Copy link

Gentlemen:

If I may, in this particular case, what is the setback of having allow-other="no"?

I mean, I have a corresponding expect WARNING constraint that explicitly tells users that they may provide their own values. Subsequently, my current setup works as a suggestion, not as an error.

In short, can't we use allow-other="no", and everybody's happy? Does it make sense?

@brian-ruf
Copy link
Contributor Author

@DimitriZhurkin as I said above, allow-others='no' is semantically and factually incorrect.

After further discussion with @aj-stein-gsa, we agree this needs to be implemented with allow-others='yes'.
Please simply do not define/perform a negative test for this. We will not produce any warning nor error.

Please remember metaschema is a specification beyond just constraint enforcement. It is also a specification to tools for other aspects of OSCAL processing. This is one of those situations where allowed-values is for other aspects of processing and not for constraint enforcement. In this case, it is only to increase consistency of values for the choices we can reasonably predict. But the specification needs to signal to tools that other values may be used when there is a choice we didn't reasonably predict. There is ample precedence for this in the core OSCAL specification.

Thank you for the questions, and for bringing this into alignment with what was defined in the task.

@DimitriZhurkin
Copy link

@brian-ruf,

When I set allow-other="yes" and provide only the PASS unit test, I'm getting the following error:

Scenario: Ensuring full test coverage for "connection-security" # features/fedramp_extensions.feature:42
   ✖ Then I should have both FAIL and PASS tests for constraint ID "connection-security" # file:/mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:466
       AssertionError: Constraint connection-security is missing a negative test

@brian-ruf
Copy link
Contributor Author

@aj-stein-gsa can you help with this? See Dimitri's comment above?

@aj-stein-gsa
Copy link
Contributor

@aj-stein-gsa can you help with this? See Dimitri's comment above?

There are two ways we can handle it: we can exclude it from the feature list or code-in a bypass, I will look at the test harness code.

@aj-stein-gsa
Copy link
Contributor

@DimitriZhurkin just to make clear our discussion during standup, can you work on this approach for the negative test. Thanks to @wandmagic, sorry I spaced on this.

# driver for the connection-security-or-whatever unit test
test-case:
  name: Test Invalid Connection Security Use
  description: This test case is to explicitly document that this allowed-values constraint cannot have a negative fail test because it allows other values.
  expectations: # check the constraint result
  - constraint-id: connection-security-or-whatever
    fail_count:
      type: "exact"
      value: 0

@DimitriZhurkin
Copy link

Unfortunately, that did not work.

After quite a bit of experimentation, this did work:

# Driver for the negative connection-security constraint unit test.
test-case:
  name: The negative connection-security constraint unit test.
  description: This test case suppresses the negative test for the connection-security "allowed-values" constraint because of its @allow-other="yes" attribute value.
  content: ../../../content/rev5/examples/ssp/xml/fedramp-ssp-example.oscal.xml
  expectations:
  - constraint-id: connection-security
    fail_count: 0

@aj-stein-gsa
Copy link
Contributor

Apologies I riffed off the latest one I found from looking code in develop. Aside from requiring content: it does not need, what else is wrong I cannot tell, all I can see is the exact argument is gone?

@DimitriZhurkin
Copy link

Initially, I did without content:, but I was getting the following error:

1) Scenario: Validating OSCAL documents with metaschema constraints # features/fedramp_extensions.feature:209
   ✔ Given I have Metaschema extensions documents # file:/mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:103
       | filename                            |
       | fedramp-external-allowed-values.xml |
       | fedramp-external-constraints.xml    |
       | oscal-external-constraints.xml      |
   ✔ When I process the constraint unit test "connection-security-FAIL.yaml" # file:/mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:111
   ✖ Then the constraint unit test should pass # file:/mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:120
       TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
           at join (node:path:1244:7)
           at file:///mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:139:33
           at file:///mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:7:71
           at __awaiter (file:///mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:3:12)
           at processTestCase (file:///mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:131:12)
           at World.<anonymous> (file:///mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:122:30)
           at file:///mnt/c/gsa-github-repos/fedramp-automation/features/steps/fedramp_extensions_steps.ts:7:71

DimitriZhurkin added a commit to DimitriZhurkin/fedramp-automation that referenced this issue Dec 20, 2024
@wandmagic
Copy link

i'll take a look on monday we may need to adjust the test running script

@Gabeblis Gabeblis linked a pull request Dec 30, 2024 that will close this issue
7 tasks
@Gabeblis Gabeblis moved this from 🔖 Ready to 👀 In review in FedRAMP Automation Dec 30, 2024
DimitriZhurkin added a commit to DimitriZhurkin/fedramp-automation that referenced this issue Jan 2, 2025
wandmagic pushed a commit that referenced this issue Jan 3, 2025
* Add connection-security constraint (issue #961)

* change fedramp ns to http

* Add help-url
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging a pull request may close this issue.

4 participants