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 schema validation for rabbitmq.additionalPlugins #87

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented Apr 15, 2020

This closes #82

Summary Of Changes

  • Add regex pattern validation to prevent shell injection and malformed input
  • Enforce a maximum 100 items for additionalPlugins and maximum 100
    characters length for each item; 100 characters and 100 items are just arbitrary limits that I've chosen. There is less than 40 out of the box plugins that rabbitmq 3.8.3 is shipped with. And the longest default plugin name has 73 characters.

Additional Context

  • kubebuilder validation tags 'Pattern' and 'MaxLength' must be specified on string type. In order to have the regex pattern and maximun length validation, I had to alias type 'string' as 'Plugin' to specify validations on items of the list. Related kubebuilder github issue can be found here. There is no current plan to support schema validation on list items from kubebuilder. Type alias is the recommended workaround according to the linked github issue.
  • This PR also updated the expected-template-output which is part of the rabbitmq chart tests. The rabbitmq chart tests were broken because of a helm version difference between local and CI image. Our CI image is testing with helm v2, while local installed helm is v3. I've updated our CI image to use helm v3, and also updated the the expected-template-output to work with helm v3.

Local Testing

Have ran unit, integration, and system-tests.

- regex to prevent shell injection and malformed input
- maximum 100 items for additionalPlugins and maximum 100
characters length for item
- kubebuilder validation tags 'Pattern' and 'MaxLength' must
be specified on string type; Alias type 'string' as 'Plugin'
to specify validations on items of the list
@Zerpet Zerpet self-requested a review April 15, 2020 14:28
@ChunyiLyu ChunyiLyu requested review from a team and j4mcs and removed request for a team April 15, 2020 14:35
@ChunyiLyu
Copy link
Contributor Author

@mkuratczyk tagging you since you found the security bug.

Copy link
Collaborator

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

Looks good 🙌 I deployed locally this code and I can't manage to sneak rabbitmqctl commands after a plugin, it returns a validation error, which is a much better option than silently ignoring.

Also verified the charts tests and the diff returns empty string. My machine is a Linux 🐧 so it should behave as in the pipeline.

:shipit:

@j4mcs
Copy link
Contributor

j4mcs commented Apr 15, 2020

lgtm

@ChunyiLyu ChunyiLyu merged commit bb51797 into master Apr 15, 2020
@ChunyiLyu ChunyiLyu deleted the shell-injection branch April 15, 2020 16:22
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.

Remote arbitrary code execution (shell incjection)
3 participants