globocom / huskyCI

Performing security tests inside your CI
https://huskyci.opensource.globo.com
BSD 3-Clause "New" or "Revised" License
572 stars 137 forks source link

enhancement: make security test timeout configurable via env variable #512

Closed raydwaipayan closed 1 year ago

raydwaipayan commented 4 years ago

Security Test timeouts are currently hardcoded in API's config.yml.

Provide user options to use a environment variable HUSKYCI_CLIENT_TESTS_TIMEOUT in the client code. All requests send to the API henceforth will contain that timeout.

On the API side, if a non zero custom timeout is detected, it shall override the default timeouts for the security tests.

Signed-off-by: Dwaipayan Ray dwaipayanray1@gmail.com

Description

Adds support for setting Security Test timeout from the client side using environment variables.

Closes #509

Proposed Changes

Add custom timeout handling logic in both the api and client. Timeout is completely optional and no function signatures were changed. So, there are no breaking changes.

Testing

make install
export HUSKYCI_CLIENT_TESTS_TIMEOUT=1
make run-client-linux
raydwaipayan commented 4 years ago

@rafaveira3 Review please! :)

raydwaipayan commented 4 years ago

Hi @rafaveira3, I think the timeout in huskydocker.go doesn't work.

The waitContainer function in api.go takes the timeOutInSeconds variable, but does nothing with it.

Is it perhaps an unimplemented feature?

rafaveira3 commented 4 years ago

Indeed, @raydwaipayan! You are totally right. You rock! 🚀

Unfortunately, we are not using this feature with this input yet. We need to work on this later on. 😅

However, for this particular issue, we need to provide our clients (not the API) the ability to set the timeout on their side. We kinda did this (hard-coded at this moment) using the timeout command here.

I was wondering if could use the same logic we are using to handle this %GIT_PRIVATE_SSH_KEY% in the API side. What do you think?

raydwaipayan commented 4 years ago

@rafaveira3 Since the docker api is being called directly by the program, I think we can't use the timeout command after the api server is started. Probably we have to use something which is supplied by docker's api. Or write our own timeout handler that is.

So the best bet is to either harcode everything or just fix the docker's timeout handling. You could open an issue on that. After it's done we could check this again and it should probably run fine :)

Krlier commented 4 years ago

@raydwaipayan, I believe that what @rafaveira3 is trying to say is that we can substitute the timeout argument in the config.yml file for each new container being started. This could be pretty much the same way as the %GIT_PRIVATE_SSH_KEY% variable.

In this scenario, we should create another method (HandleTimeoutSubstitution()?), such as this one, to handle the timeout substitution, making it configurable through an environment variable to be supplied by the client.

Other than that, we should also do a timeout error handling, such as this one.

After that, another change you may need to do is to add some logic, similar to this one, so that the security test can handle properly the timeout output.

What do you say we start adding these changes by replacing how GitLeaks does it since it's already hard coded? 🙂

raydwaipayan commented 4 years ago

@Krlier True I looked into it and it seems doable. I will try to draw something up :)

Krlier commented 3 years ago

@Krlier True I looked into it and it seems doable. I will try to draw something up :)

Hey, @raydwaipayan!

How's it going? Did you come up with something?

Do you need any help?