-
Notifications
You must be signed in to change notification settings - Fork 9
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
enable GIN mTLS support #7
Conversation
@alvinlee001 nice! can we make the |
Hi @malwaredllc, are u saying that you just want it to be https disabled or enabled? If https enabled, implies opting for mTLS? What about opting for TLS but not mTLS? It totally depends on you, but right now TLS is mandatory, while mTLS is optional. |
@alvinlee001 Basically TLS should be optional. The problem I want to solve is that right now, if a server's hostname or IP does not appear in this TLS certificate config file here, then it will be unable to communicate with other servers. This means that unless all of your servers are in the same subdomain that you can use a wildcard for (i.e. For example, if I want to add One way around this is to simply not use TLS (make it optional) to not have to worry about this, so the cluster size can dynamically change without issue. Alternatively, I need to figure out a more flexible way of doing TLS. |
I see, @malwaredllc can we have an additional property called |
@alvinlee001 Yeah I think |
Hi @malwaredllc, i have implemented it. There will be a flag in CLI and json to specify if https should be enabled, it is enabled by default unless specified otherwise. I have pushed the new commit, kindly have a look. |
@alvinlee001 thanks for your help with this, looks like the build is failing though (see build tests ran on this PR), can you take a look? |
Ok wait |
… enable or disable HTTPS
Hi @malwaredllc , sorry I have force pushed again, missed some changes previously. Now the tests are passing. |
@alvinlee001 Looks good! Last thing will be to add a test to make sure that HTTP without TLS works properly (you can basically copy/paste this test in |
Alright, sure @malwaredllc |
hi @malwaredllc , added test and it is passing, previously has been testing the HTTP rest locally as well, it is working |
@@ -59,7 +61,7 @@ func HashId(key string) uint32 { | |||
// Load nodes config file | |||
func LoadNodesConfig(config_file string) NodesConfig { | |||
file, _ := ioutil.ReadFile(config_file) | |||
nodes_config := NodesConfig{} | |||
nodes_config := NodesConfig{EnableClientAuth: true, EnableHttps: true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question, why are EnableClientAuth
and EnableHttps
hard coded to true
here? Shouldn't these values come from the config file or command line flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @malwaredllc, if unspecified, these will be the default values, meaning if the config have the fields to be absent, it will have https and mTLS as enabled by default
@alvinlee001 looks awesome thanks for doing that! I left a couple of comments on small things to address above but besides that it looks ready to go |
Hi @malwaredllc , I resolved the 2 comments just now, and pushed changes. pls have a look |
RELATIVE_CONFIG_PATH = "configs/nodes-local.json" | ||
RELATIVE_CLIENT_CERT_DIR = "certs" | ||
RELATIVE_CONFIG_PATH = "configs/nodes-local.json" | ||
RELATIVE_CONFIG_PATH_HTTP = "configs/nodes-local-insecure.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this file to the commit
@alvinlee001 after further investigation, it appears the integration tests are passing but they shouldn't be (see here: https://github.com/malwaredllc/minicache/runs/6465112038?check_suite_focus=true#step:8:33) - it is unable to connect to any nodes now, it visits them all then loops forever because it is skipping visited nodes - we can fix this test but still the main issue is that the connections to the server are failing |
I am fixing the issue causing the test to appear to pass here and will merge it momentarily |
@alvinlee001 ok I fixed the issue causing the test to appear to pass when it should fail. You can merge in the latest changes from the main branch |
Hi @malwaredllc , sorry for the late reply, I was busy with my day job. I tried to find the issue with my code, which I think there are a few, and fixed them... but the tests still fail. However, if I run each of the tests individually by commenting out the others, all tests pass... I am not sure why though. |
@alvinlee001 No worries, if you look at the test output here you'lll see the test failing is
So this indicates either the client or server is still trying to use TLS, but one side isn't. I think it is may be because the config file you are trying to use |
I added a print statement which revealed "enable_https" and "enable_client_auth" are both set to true in the test
|
I figured out the problem, the I am fixing it now and will push changes to your branch here. |
@alvinlee001 I'm not able to modify your PR directly so I made a PR with your changes + the fix I added: #9 Either you can copy over the changes to your branch and we can merge it (so you get credit for your contribution), or we can merge that PR. |
Hi @malwaredllc , I have patched my changes with yours. U may merge this PR. Thanks |
client/cache_client/cache_client.go
Outdated
@@ -257,7 +276,7 @@ func (c *ClientWrapper) Put(key string, value string) error { | |||
// check response | |||
res, err := new(http.Client).Do(req) | |||
defer res.Body.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alvinlee001 I think this line is causing the test to fail with a segfault. The error check needs to go before using the res object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wokay, done pushing the fix.
Merged! Thanks for the contribution, feel free to add any ideas for other features or improvements |
I also just updated the setup/usage information in the readme to include these new changes: https://github.com/malwaredllc/minicache#set-up-and-usage |
pull request for #6