Skip to content

Document threat models #14976

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

Merged
merged 17 commits into from
Dec 4, 2023
Merged

Document threat models #14976

merged 17 commits into from
Dec 4, 2023

Conversation

saritai
Copy link
Contributor

@saritai saritai commented Nov 30, 2023

This pull request addresses https://github.com/github/docs-content/issues/12431.

Release issue: https://github.com/github/releases/issues/3545

Reviewers: I'd like to know if the information I included is accurate, is enough detail, and is not missing anything. I put it in a new separate section of the article, since these extensible predicates are part of the separate threat-modelsshared library pack. I'll clean up some of the language/structure later but just wanted to make sure I am on the right track.

@smowton smowton changed the title add info Document threat models Nov 30, 2023
@saritai saritai marked this pull request as ready for review November 30, 2023 22:43
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up.

I think it's important to link the threat model kind to the 8th value in the sourceModel and mention that if environment is enabled, then all source models of the environment kind will be included in the analysis. Otherwise, they will be excluded.

In the example you have:

       - [local, true, 0]
       - [environment, false, 1]

All local threat models will be enabled, except for environment variables threat sources, which are disabled. I think the fact that there is a hierarchy of threat models is important to get across as well here.

Copy link
Contributor

@jf205 jf205 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR @saritai. The internal doc you got the information from contains quite low-level detail so I've tried to make the new section easier for external users to understand. I pushed the changes directly as it was easier than making suggestions on the PR but please feel free to edit my additions as much as you like ❤️

- ``remote`` which represents remote HTTP requests.
- ``local`` which represents data from local files (``file``), command-line arguments (``commandargs``), database reads (``database``), and environment variables(``environement``).

When running a CodeQL analysis, the ``remote`` threat model is included by default. You can optionally include other threat models as appropriate when using the CodeQL CLI and in GitHub code scanning. For more information see TODO.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you add links to the information about using threat models with the CLI and in code scanning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added links based on the new sections in the docs-internal PR. However, I am not sure if the names of those sections are set in stone, so will want to wait till Monday to confirm before we merge this PR.

Also good to note that right now, those links just lead to the top level of the article since the sections don't exist. But doesn't seem to lead to any 404s (since the articles themselves already exist, the sections just don't) so should be alright to keep them in there (and these docs will be published after the docs-internal PR).

@jf205
Copy link
Contributor

jf205 commented Dec 1, 2023

Thanks for writing this up.

I think it's important to link the threat model kind to the 8th value in the sourceModel and mention that if environment is enabled, then all source models of the environment kind will be included in the analysis. Otherwise, they will be excluded.

In the example you have:

       - [local, true, 0]
       - [environment, false, 1]

All local threat models will be enabled, except for environment variables threat sources, which are disabled. I think the fact that there is a hierarchy of threat models is important to get across as well here.

@aeisenberg do you think we need to talk to this level of detail? It seems more appropriate for our internal authors. I think we can talk about what threat models are in relation to the sourceModel predicate and then describe (or link to information) about how they are used to customize analyses with the CLI and in code scanning. Mentioning their implementation with data extensions seems out of scope for this release.

saritai and others added 4 commits December 1, 2023 11:32
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
aeisenberg
aeisenberg previously approved these changes Dec 1, 2023
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me.

Currently, this applies to java-kotlin only, but threat models are being rolled out to the other languages as well. Each language should use the same threat kinds. Is the plan to document each language individually or merge the parts of the documentation that are shared?

Either way, I think what you have here is fine for now.

@saritai
Copy link
Contributor Author

saritai commented Dec 1, 2023

@aeisenberg Thanks for the review! And great point about this coming to other languages. We could definitely make part of this section a reusable and then use that for the other languages, but I suppose we can just do that once the next language comes up 👍

I'll just wait till Monday to merge this PR because I want to make sure the links I have to the CodeQL CLI and code scanning docs are correct (not sure if they will change).

subatoi
subatoi previously approved these changes Dec 4, 2023
Copy link
Contributor

@subatoi subatoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one tiny typo that seems to have already been there: happy to look again and approve after SMEs have taken a last look, if needed!

Thanks again for writing this up, Sarita! ✨

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>
subatoi
subatoi previously approved these changes Dec 4, 2023
@saritai saritai merged commit 5a4ea77 into main Dec 4, 2023
@saritai saritai deleted the saritai/docs-update-12431 branch December 4, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants