Add initial GitHub integration #422

hwine · 2020-12-03T20:33:53Z

Propose a squash merge on this -- or I can reorder & clean up some for a real review.

This branch is already running in jenkins staging without issues. All changes (except as noted) are in the github/ directory, so do not impact other code.

Changes outside of github/ are:

Makefile - re-introduction of python version cross check (can be reverted), and some refactoring of finding all python files (could be improved)
conftest.py - added in GitHub client, enhanced metadata handling as discussed
setup.py - fix for document generation error
tox.ini - added, but not used in Makefile yet, so can be reverted
cache.py, service_report_generator.py, aws/* (2 files), gcp/* (1 file) - impact of running pyupgrade in my tree, single line syntax change, can be reverted
adding bandit to pre-commit checks, with baseline for the one false positive it found
adding other hygiene checks to pre-commit which all pass
updated secrets baseline, as we no longer have false positives under docs/_build

Needed for pytest to find local `conftest.py` file.

needs frost integration

now can run from any directory

The deleted filew were for testing in dev. Also a few changes for python3.6 support.

Also added the github/orgs code

Fortunately, fat-fingering a token caused the error paths to be executed, pointing to some scoping issues. Resolved.

In general works through getting minimal set of JSON to process. About to move standard being tested into the parametrization id

This now does not break `make doctest` Also cleaned up comments, added todo's and license

Python >3.6 doesn't work with dataclasses 0.7, which made travis runs fail. dataclasses 0.6 doesn't fail. Added tox.ini to easily test this. N.B. `tox` is expected to be installed globally

Just picked this up in a rebase -- no idea why it's there.

This tests the `foo.retrieve_github_data` calls as a standalone script. Goal is to have the script + GitHub data importable as a package. First priority is to have working in frost, though.

Also sets stage to have cli generate csv files for upload to S3 and possible input to frost.

Good enough output now to verify using frost from Jenkins

The condition of "no protection rules" was not being tested properly.

The hack wasn't required at this location, only in main(). But we're still called during doctests with token=None. Handle that.

the v4 & v3 id's are immutable, names are not

And some minor code tweaks to pass black & dectect-secrets Also fixed some potential shell issues

Work done by pyupgrade pre-commit plugin. Included: - conversion to f-strings where possible - removal of redundant syntax - application of Exception hierarchy changes in py 3.3 (IOError is now a subclass of OSError, and need not be listed separately) This was done to get a baseline before updating to py38 syntax

Commit dcac1cd broke this by performing a dereference at an earlier stage, and not removing the (now incorrect) later dereference.

It was having trouble with "nbhooks", but we don't need that

Make is much simpler to retrieve data from the JSON output.

The change to make METADATA_KEYS a set means we can only have one definition of the Criteria class. It also eliminates some duplicate code.

Latest master this time :/

ajvb

Couple comments from my first quick pass.

I really think that some tests should be added, as there is a lot of code here. As well, there are a lot of TODO comments. Maybe not all of them need to be addressed before merging, but if they don't need to be fixed now then maybe we can convert them into follow-up issues and remove the TODO comment?

Also, what exactly is the github/github_schema.py file?

Makefile

cache.py

conftest.py

github/.gitignore

ajvb · 2020-12-03T22:43:23Z

github/exemptions-github.yaml

@@ -0,0 +1,10 @@
+exemptions:


I believe this should be moved to foxsec-automation.

Agreed, but then we're saying this can't land until I finish the plumbing in foxsec-automation. A bit of chicken and egg. An option would be to make an issue to move this. ATM, it just has a "test exemption" -- it does have to be moved before any real data is entered.

Another discussion point -- the modern solution is a mono-repo, but we can't do that. :)

github/run.sh

hwine · 2020-12-04T01:33:27Z

I really think that some tests should be added, as there is a lot of code here. As well, there are a lot of TODO

So, this the tension between "land early incomplete, but not buggy" and "wait until it's perfect". The longer it live outside of the production branch, the more cruft may show up. We can discuss.

Also, what exactly is the github/github_schema.py file?

Ah, yes -- that is an autogenerated file by a tool in the graphQL library. Anytime we want to use a newer feature of the GitHub GraphQL API, we need to regenerate this file. That clearly needs some docs, but we don't want to regenerate it at packaging time, as we need a full test suite run.

To clarify the status of the code - it runs, and produces correct output which is stored for metrics usage. It does not yet consume the frost results and take actions. Think of it as the MVP, with more sprints to come.

We can discuss how we handle all this at the frost meetup

hwine · 2020-12-04T16:59:40Z

Also, what exactly is the github/github_schema.py file?

That clearly needs some docs, ...

Updated github/README.md in next commit

ajvb · 2020-12-04T19:16:13Z

github/manage_issues.py

+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at https://mozilla.org/MPL/2.0/.
+"""Perform actions based on frost results."""


Consider moving to foxsec-automation

agreed for now

ajvb · 2020-12-04T19:16:22Z

github/messages-github.yaml

@@ -0,0 +1,209 @@
+# YAML file for GitHub issue templates


Consider moving to foxsec-automation

ajvb

Second pass through.

github/branches/test_branch_protection.py

ajvb · 2020-12-04T21:20:56Z

github/branches/test_branch_protection.py

+import pytest
+
+
+def idfn(val: Any) -> Optional[str]:


NB: Should this be alongside repos_to_check()? And called get_repo_id or similar?

My thinking had been "repos_to_check()" is a pure GitHub thing. "idfns=" is a pytest thing.

I think I can move it to the "BranchOfInterest" class, as I did for "Criteria", unless that feels "too different".

ajvb · 2020-12-04T21:26:08Z

github/branches/validate_compliance.py

+            f"ERROR:SOGH001:{data.name_with_owner}:{branch} has no {criteria} rule"
+        )
+        return meets_criteria(active_rules, criteria), message
+        # vscode correctly tells me that all code below here is unreachable


Why keep unreachable code?

Because I had an inkling that this would need refactoring -- working on that on another branch

ajvb · 2020-12-04T21:26:28Z

github/branches/validate_compliance.py

+
+    if not active_rules:
+        # results.append(
+        assert (


should this be returning rather than asserting?

Maybe -- part of the refactor, leaving open

ajvb · 2020-12-04T21:27:44Z

github/branches/validate_compliance.py

+    return met
+
+
+def validate_branch_protections(


Would love to see a doctest for this func

Coming soon in (another) branch near you. This is the part I may have to refactor heavily as I automate issue generation.

Leaving this open for now, I'll (hopefully) be able to pull in a solution.

ajvb · 2020-12-04T21:40:24Z

github/orgs/retrieve_github_data.py

+    ## csv_out.writerow(OrgInfo.csv_header())
+    ## for org in args.orgs:
+    ##     row_data = get_org_info(endpoint, org)
+    ##     csv_output(row_data, csv_writer=csv_out)


should this be removed or added behind a new cli opt?

Good question -- probably the cli prompt (TIL).

The intent is to display a more human readable format when run from the cli

ajvb · 2020-12-04T21:41:37Z

github/orgs/retrieve_github_data.py

+        *in_files,
+    ]
+    # python 3.6 doesn't support capture_output
+    status = subprocess.run(cmd, capture_output=True)  # nosec


Why not just read the json files and parse them within Python?

because 'jq' is more expressive, and easier to change? (<== my reasons during development)

I'd prefer to move to "jq bindings", and get rid of the "subprocess" ugliness, unless that still raises the same concerns.

@ajvb thoughts?

I'd definitely prefer to use the python std lib over requiring another dependency, especially an additional system package. As well, though it is more expressive, it requires any new developer to learn how to use jq without providing what seems to me to be enough value to justify that.

Yeah - there seems to be a great debate on whether "reading jq" is required skill, and whether "jq" should be a standard tool in images. I vote "yes" on both, but ulfr switched ("yes" when I first started, then "meh" later)

I'll rewrite

github/orgs/conftest.py

ajvb · 2020-12-04T21:45:21Z

github/branches/retrieve_github_data.py

+    # real work
+    # DEV_HACK: find better way to insert dev default
+    path_to_metadata = os.environ.setdefault(
+        "PATH_TO_METADATA", "~/repos/foxsec/master/services/metadata"


Should probably provide this path as an option?

And, I think an option to the groovy pipeline.

github/branches/retrieve_github_data.py

Mostly cleanup. Remaining TODO items will be done prior to production.

I sure hope I remember to squash these

hwine added 30 commits August 27, 2020 10:18

Make directory a package

5dc6721

Needed for pytest to find local `conftest.py` file.

working via pytest

18ef0fa

needs frost integration

convert to relative imports

5cbdc18

now can run from any directory

running fine against all data

5057612

Clean up unneeded files

a81fb21

The deleted filew were for testing in dev. Also a few changes for python3.6 support.

Clean up integration into frost

fcbf23e

Refactored to github/{branches,orgs}

8b5a16e

Also added the github/orgs code

Fix error reporting

b4fa324

Fortunately, fat-fingering a token caused the error paths to be executed, pointing to some scoping issues. Resolved.

WIP: pre move stdname

8815245

In general works through getting minimal set of JSON to process. About to move standard being tested into the parametrization id

offline working, misc cleanup

466084a

This now does not break `make doctest` Also cleaned up comments, added todo's and license

Add tox.ini to perform doctests and fixed 3.8

a451634

Python >3.6 doesn't work with dataclasses 0.7, which made travis runs fail. dataclasses 0.6 doesn't fail. Added tox.ini to easily test this. N.B. `tox` is expected to be installed globally

Remove rg (ripgrep) as not used & fails on travis

85cc75c

Just picked this up in a rebase -- no idea why it's there.

wip before refactor

165f48e

Temp crutch to ensure we have a cli at end

66dca41

This tests the `foo.retrieve_github_data` calls as a standalone script. Goal is to have the script + GitHub data importable as a package. First priority is to have working in frost, though.

Refactored for better cli behavior

ca2d495

Also sets stage to have cli generate csv files for upload to S3 and possible input to frost.

wrapper.py no longer breaks make doctest

f325c35

Add action stubs

7edca3e

Good enough output now to verify using frost from Jenkins

Add missing required criteria

1ea32d2

The condition of "no protection rules" was not being tested properly.

Fix doctest bustage

9b1bd13

Provide valid connection under pytest

2720017

The hack wasn't required at this location, only in main(). But we're still called during doctests with token=None. Handle that.

Add v3 & v4 ids for branch data

dcac1cd

the v4 & v3 id's are immutable, names are not

wip

e83483b

wip

54ab974

wip

9d58673

wip

0738929

wip

1023c67

Add pre-commit hooks

c341d4e

And some minor code tweaks to pass black & dectect-secrets Also fixed some potential shell issues

Fix "no attribute" Exception

7994767

Commit dcac1cd broke this by performing a dereference at an earlier stage, and not removing the (now incorrect) later dereference.

Fix pre-commit versions to run on travis

8127155

It was having trouble with "nbhooks", but we don't need that

hwine added 10 commits October 26, 2020 09:16

METADATA_KEYS can be a set

35ef8ff

Change parameterization to return dict

e778e92

Make is much simpler to retrieve data from the JSON output.

remove unused code

a0d59c8

Various cleanup, prior to merging master

1e5ff7b

Merge branch 'master' into merge_master_branch_check

6e65f23

Relocate 'Criteria' to helpers.py

115c9d1

The change to make METADATA_KEYS a set means we can only have one definition of the Criteria class. It also eliminates some duplicate code.

Fix bustage

c12b8d6

Merge branch 'master' into branch_check

bf55b62

Latest master this time :/

Don't run code that isn't ready to run

e092802

Remove false positives in deleted files

b7deb35

hwine added enhancement github labels Dec 3, 2020

hwine requested a review from ajvb December 3, 2020 20:33

hwine self-assigned this Dec 3, 2020

ajvb reviewed Dec 3, 2020

View reviewed changes

Address review comments

3994281

ajvb reviewed Dec 4, 2020

View reviewed changes

hwine added 3 commits December 7, 2020 10:32

Address more review comments

7bd547f

Mostly cleanup. Remaining TODO items will be done prior to production.

Fix doctest

550a76a

More review comments addressed.

8925ad3

ajvb added this to the 0.5.0 milestone Jan 6, 2021

Base automatically changed from master to main January 19, 2021 19:25

hwine added 4 commits August 26, 2022 13:12

Debug which token is being used :/

3df42d2

More debug

6c52188

debug

3a7f315

Debug

c7b0164

I sure hope I remember to squash these

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial GitHub integration #422

Add initial GitHub integration #422

hwine commented Dec 3, 2020

ajvb left a comment

ajvb Dec 3, 2020

hwine Dec 4, 2020

hwine commented Dec 4, 2020

hwine commented Dec 4, 2020 •

edited

Loading

ajvb Dec 4, 2020

hwine Dec 7, 2020

ajvb Dec 4, 2020

hwine Dec 7, 2020

ajvb left a comment

ajvb Dec 4, 2020

hwine Dec 7, 2020

ajvb Dec 4, 2020

hwine Dec 7, 2020

ajvb Dec 4, 2020

hwine Dec 7, 2020

ajvb Dec 4, 2020

hwine Dec 7, 2020

ajvb Dec 4, 2020

hwine Dec 7, 2020

ajvb Dec 4, 2020

hwine Dec 7, 2020

ajvb Dec 7, 2020

hwine Dec 7, 2020

ajvb Dec 4, 2020

hwine Dec 7, 2020

Add initial GitHub integration #422

Are you sure you want to change the base?

Add initial GitHub integration #422

Conversation

hwine commented Dec 3, 2020

ajvb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwine commented Dec 4, 2020

hwine commented Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajvb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwine commented Dec 4, 2020 •

edited

Loading