-
Notifications
You must be signed in to change notification settings - Fork 100
Cert file outside allowed directory in aux content #72
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
Cert file outside allowed directory in aux content #72
Conversation
…ithub.com:nginx/agent into fix-cert-outside-allowed-directory-in-aux-content
dhurley
left a comment
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 run make format lint ?
src/core/nginx.go
Outdated
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("%v", err) |
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 undo this change? I don't think its required.
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.
Sorry forgot to remove that, changed it. Ran make format lint again but there were no changes made.
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.
@aphralG I think you should make the following change to the Makefile
diff --git a/Makefile b/Makefile
index d3e3ad5..59b3c39 100644
--- a/Makefile
+++ b/Makefile
@@ -75,6 +75,7 @@ lint: ## Run linter
format: ## Format code
go fmt ./...
+ cd sdk && go fmt ./...And run make format again. I think the change can be included in your PR
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.
Thank you, that fixed it 🙂
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.
I should've mentioned that you'll need to run make deps too.
| } | ||
|
|
||
| if !isAllowed { | ||
| return fmt.Errorf("file outside allowed directories %s", file) |
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.
should not error here, the cert information should still be parsed to provide meta data, and not saved into the aux file. We don't want to error even if the cert is outside the allowed directory.
Proposed changes
Remote cert file content outside allowed directory.
When a cert file was added the contents were not checked to see if it was in an allowed directory. This caused the aux file to include files from outside the allowed directories. Which then caused attempts to update the config to fail.
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocumentREADME.md)