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

Fix license handling #2

Merged
merged 5 commits into from
Aug 29, 2024
Merged

Fix license handling #2

merged 5 commits into from
Aug 29, 2024

Conversation

ntBre
Copy link
Collaborator

@ntBre ntBre commented Aug 26, 2024

Based on my failing test run, ${{ github.workspace }} just points to the directory where the runner is running code, not to the directory of organization secrets like I thought. I took this line from here in yammbs, but I didn't notice the second OE_LICENSE section here, which actually writes the ${{ secrets.OE_LICENSE }} to a file. This PR restores my earlier OE_LICENSE handling (writing it to /tmp) and adds two further safeguards against committing the license to the repo:

  1. ignoring oe_license.txt in the .gitignore file
  2. running rm $OE_LICENSE before doing any git operations

Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

I think that running git add . at all is the bigger thing to avoid here. Would it be possible to only add the specific files we intend to commit instead?

@ntBre
Copy link
Collaborator Author

ntBre commented Aug 26, 2024

It's a little tricky to do that because the current workflow only has access to the path to the YAML input file, not to the output directory itself. The python script assumes the output should go in the same directory as the YAML file, though, so I guess that's a safe assumption for the shell code too. Working on that change now!

there are three assumptions encoded here that are shared with `main.py`. first,
the output is expected to be written in the same directory as the input YAML
file; second, the output subdirectory is expected to be called `output`; and
third, the desired output files are `dde.csv`, `icrmsd.csv`, `rmsd.csv`, and
`tfd.csv`
@ntBre
Copy link
Collaborator Author

ntBre commented Aug 26, 2024

Good call on that. As written, I think it also would have tried to add the output SQLite file, which I had ignored in my first draft of this repo but not in this one.

@ntBre ntBre requested a review from j-wags August 27, 2024 20:03
Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

LGMT % refreshing my understanding of how the secret is handled

shell: bash -l {0}
run: |
set -e
echo "$LICENSE" > /tmp/oe_license.txt
Copy link
Member

Choose a reason for hiding this comment

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

Just double-checking my understanding, this spits out the license to file but it's encrypted, or otherwise not accessible from a third-party attacker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review! Yeah, at least to my understanding, this writes the license to a plain file, but it should be restricted to our instance of the GitHub runner, although it's not encrypted.

The only difference between this and the yammbs CI is where the secret is stored. In yammbs it's ${{ github.workspace }}/oe_license.txt, which expands to /home/runner/work/yammbs/yammbs/oe_license.txt. I chose /tmp here to keep it out of the repo in case of any errant git add commands. Of course, with the gitignore and removing the license after using it, it should be safer to write it somewhere else if you prefer. $HOME could be another option, if we're worried specifically about /tmp.

Copy link
Member

Choose a reason for hiding this comment

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

Yup all good ideas I think

@ntBre ntBre merged commit a4dd5c1 into master Aug 29, 2024
@ntBre ntBre deleted the fix-license branch August 29, 2024 17:01
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