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

Add support for server side hooks #80

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

Conversation

ajjl
Copy link

@ajjl ajjl commented May 15, 2018

Client side hooks are great, but rely on developers properly setting
them up and not taking shortcuts. Server side hooks provide an
opportunity to enforce security policies at a more global level.

This commit adds an "update_hook" option which can be added as a
serverside update hook. It scans the pushed commits for secrets.

Fixes issue: #79

@ajjl ajjl changed the title Add support for server side hooks wip: Add support for server side hooks May 15, 2018
@ajjl
Copy link
Author

ajjl commented May 15, 2018

Please let me know what if anything, I should do for tests.

@ajjl ajjl force-pushed the feature/Add-serverside-hook branch from ed2d1d2 to d15da21 Compare May 15, 2018 20:31
@ajjl ajjl changed the title wip: Add support for server side hooks Add support for server side hooks May 15, 2018
@bjbishop
Copy link

This would be really useful! Thanks @ajjl

@ramya-ravula-ctr
Copy link

can we enforce this on public github or it is only for github enterprise appliance ?

@ajjl
Copy link
Author

ajjl commented May 22, 2018

@ramya-ravula-ctr I dont think this works on publicly hosted github. I don't know about github enterprise is set up but I imagine it would work. This is meant for a git server that you have control over. An example would be a self hosted gitlab server.

@ramya-ravula-ctr
Copy link

Thanks @ajjl for the response. i figured out it's not for publicly hosted github and public github only supports client side hooks.

@ajjl
Copy link
Author

ajjl commented May 30, 2018

@mtdowling any thoughts on this feature?

@ajjl
Copy link
Author

ajjl commented Jun 28, 2018

anybody... @mtdowling ?

@rix0rrr
Copy link

rix0rrr commented Jul 17, 2018

I was looking into this as well, and was wondering the following:

Where do you store your "secret patterns"?

Normally they get stored in .git/config, but that file does not get shared between clones. So where does the server get the list of prohibited patterns?

@ajjl
Copy link
Author

ajjl commented Jul 19, 2018

Hi @rix0rrr
Using gitlab I have the secrets in the .gitconfig file in the home folder of the git user on the gitlab server. I am not sure how it would be set up in other environments

@mtdowling
Copy link
Contributor

Sorry for the delay! I was on paternity leave and then dropped the ball on reviewing this.

I've left some comments on the review.

git-secrets Show resolved Hide resolved
git-secrets Show resolved Hide resolved
git-secrets Show resolved Hide resolved
@ajjl
Copy link
Author

ajjl commented Aug 10, 2018

Thanks for the review @mtdowling! I will take a look at this this weekend or next week and get back to you.

@ajjl
Copy link
Author

ajjl commented Aug 10, 2018

Also congrats on your new/bigger family! @mtdowling

uleinal and others added 2 commits October 1, 2018 10:56
Client side hooks are great, but rely on developers properly setting
them up and not taking shortcuts. Server side hooks provide an
opportunity to enforce security policies at a more global level.

This commit adds an "update_hook" option which can be added as a
serverside update hook. It scans the pushed commits for secrets.
@ajjl ajjl force-pushed the feature/Add-serverside-hook branch from d15da21 to 38b48c3 Compare October 1, 2018 15:57
@uleinal
Copy link

uleinal commented Oct 1, 2018

Wow, I totally lost track of this, anyways, I added the comment into the code, and rebased the branch. Let me know if you want anymore changes before merging @mtdowling

@ajjl
Copy link
Author

ajjl commented Oct 1, 2018

That was me up there logged into my work account. Hopefully not too confusing.

@mtdowling
Copy link
Contributor

Everything is looking good here. The only thing that I think is missing is tests. Is that something you can add?

@sparr
Copy link
Contributor

sparr commented Jun 16, 2023

I think this is ready along with the tests in #204.

@sparr sparr dismissed mtdowling’s stale review June 20, 2023 23:35

All comments resolved.

@sparr sparr closed this Jun 20, 2023
@sparr sparr reopened this Jun 20, 2023
@sparr
Copy link
Contributor

sparr commented Jun 20, 2023

Close and reopen to trigger tests

Copy link
Contributor

@sparr sparr left a comment

Choose a reason for hiding this comment

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

LGTM.
Confirming that new tests in #204 pass.

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.

7 participants