You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@ambari.apache.org by Vitalyi Brodetskyi <vb...@hortonworks.com> on 2017/03/14 22:35:18 UTC
Review Request 57625: Minor refactoring and clean up in ambari-server
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57625/
-----------------------------------------------------------
Review request for Ambari, Robert Levas, Sumit Mohanty, Sid Wagle, and Yusaku Sako.
Bugs: AMBARI-20453
https://issues.apache.org/jira/browse/AMBARI-20453
Repository: ambari
Description
-------
Minor refactoring and clean up in ambari-server
Diffs
-----
ambari-server/src/main/assemblies/server.xml 768ba68
ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java 8d54acb
ambari-server/src/main/package/rpm/postinstall.sh 1e8e0f0
ambari-server/src/main/python/ambari_server/resourceFilesKeeper.py 188f3ff
ambari-server/src/main/python/ambari_server/serverConfiguration.py 3dd165b
ambari-server/src/main/resources/scripts/check_ambari_permissions.py PRE-CREATION
Diff: https://reviews.apache.org/r/57625/diff/1/
Testing
-------
mvn clean test
Thanks,
Vitalyi Brodetskyi
Re: Review Request 57625: Minor refactoring and clean up in
ambari-server
Posted by Robert Levas <rl...@hortonworks.com>.
> On March 15, 2017, 9:51 a.m., Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java
> > Lines 166-167 (patched)
> > <https://reviews.apache.org/r/57625/diff/1/?file=1664892#file1664892line166>
> >
> > This is really dangerous and could be considered a security issue. Same with the previous `runcommand` calls. We need to see what happens if `security.server.keys_dir` is set to something like
> >
> > ```
> > ;touch /tmp/security_issue;
> > ```
>
> Vitalyi Brodetskyi wrote:
> Robert, this code works for a long time in that way, so i think it's not urgent. Maybe we can create separate jira to check how it works with "touch /tmp/security_issue" and try to fix it in next release? Sumit FYI
I tested Oracle Java and Open JDK version 1.7 and 1.8. All 4 JVMs tokenize the command string and this prevents any injection. I am dropping this issue.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57625/#review169004
-----------------------------------------------------------
On March 14, 2017, 6:35 p.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57625/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 6:35 p.m.)
>
>
> Review request for Ambari, Robert Levas, Sumit Mohanty, Sid Wagle, and Yusaku Sako.
>
>
> Bugs: AMBARI-20453
> https://issues.apache.org/jira/browse/AMBARI-20453
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Minor refactoring and clean up in ambari-server
>
>
> Diffs
> -----
>
> ambari-server/src/main/assemblies/server.xml 768ba68
> ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java 8d54acb
> ambari-server/src/main/package/rpm/postinstall.sh 1e8e0f0
> ambari-server/src/main/python/ambari_server/resourceFilesKeeper.py 188f3ff
> ambari-server/src/main/python/ambari_server/serverConfiguration.py 3dd165b
> ambari-server/src/main/resources/scripts/check_ambari_permissions.py PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/57625/diff/1/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>
Re: Review Request 57625: Minor refactoring and clean up in
ambari-server
Posted by Vitalyi Brodetskyi <vb...@hortonworks.com>.
> On \u0411\u0435\u0440\u0435\u0437\u0435\u043d\u044c 15, 2017, 1:51 \u043f\u0456\u0441\u043b\u044f \u043f\u043e\u043b\u0443\u0434\u043d\u044f, Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java
> > Lines 166-167 (patched)
> > <https://reviews.apache.org/r/57625/diff/1/?file=1664892#file1664892line166>
> >
> > This is really dangerous and could be considered a security issue. Same with the previous `runcommand` calls. We need to see what happens if `security.server.keys_dir` is set to something like
> >
> > ```
> > ;touch /tmp/security_issue;
> > ```
Robert, this code works for a long time in that way, so i think it's not urgent. Maybe we can create separate jira to check how it works with "touch /tmp/security_issue" and try to fix it in next release? Sumit FYI
- Vitalyi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57625/#review169004
-----------------------------------------------------------
On \u0411\u0435\u0440\u0435\u0437\u0435\u043d\u044c 14, 2017, 10:35 \u043f\u0456\u0441\u043b\u044f \u043f\u043e\u043b\u0443\u0434\u043d\u044f, Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57625/
> -----------------------------------------------------------
>
> (Updated \u0411\u0435\u0440\u0435\u0437\u0435\u043d\u044c 14, 2017, 10:35 \u043f\u0456\u0441\u043b\u044f \u043f\u043e\u043b\u0443\u0434\u043d\u044f)
>
>
> Review request for Ambari, Robert Levas, Sumit Mohanty, Sid Wagle, and Yusaku Sako.
>
>
> Bugs: AMBARI-20453
> https://issues.apache.org/jira/browse/AMBARI-20453
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Minor refactoring and clean up in ambari-server
>
>
> Diffs
> -----
>
> ambari-server/src/main/assemblies/server.xml 768ba68
> ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java 8d54acb
> ambari-server/src/main/package/rpm/postinstall.sh 1e8e0f0
> ambari-server/src/main/python/ambari_server/resourceFilesKeeper.py 188f3ff
> ambari-server/src/main/python/ambari_server/serverConfiguration.py 3dd165b
> ambari-server/src/main/resources/scripts/check_ambari_permissions.py PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/57625/diff/1/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>
Re: Review Request 57625: Minor refactoring and clean up in
ambari-server
Posted by Robert Levas <rl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57625/#review169004
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
ambari-server/src/main/assemblies/server.xml
Lines 376 (patched)
<https://reviews.apache.org/r/57625/#comment241332>
This could be '644' since it is a txt file and does not need to be executable. But 755 will suffice. Maybe if you make other changes and you remember, you change this.
ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java
Lines 166-167 (patched)
<https://reviews.apache.org/r/57625/#comment241333>
This is really dangerous and could be considered a security issue. Same with the previous `runcommand` calls. We need to see what happens if `security.server.keys_dir` is set to something like
```
;touch /tmp/security_issue;
```
- Robert Levas
On March 14, 2017, 6:35 p.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57625/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 6:35 p.m.)
>
>
> Review request for Ambari, Robert Levas, Sumit Mohanty, Sid Wagle, and Yusaku Sako.
>
>
> Bugs: AMBARI-20453
> https://issues.apache.org/jira/browse/AMBARI-20453
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Minor refactoring and clean up in ambari-server
>
>
> Diffs
> -----
>
> ambari-server/src/main/assemblies/server.xml 768ba68
> ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java 8d54acb
> ambari-server/src/main/package/rpm/postinstall.sh 1e8e0f0
> ambari-server/src/main/python/ambari_server/resourceFilesKeeper.py 188f3ff
> ambari-server/src/main/python/ambari_server/serverConfiguration.py 3dd165b
> ambari-server/src/main/resources/scripts/check_ambari_permissions.py PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/57625/diff/1/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>
Re: Review Request 57625: Minor refactoring and clean up in
ambari-server
Posted by Sumit Mohanty <sm...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57625/#review169022
-----------------------------------------------------------
Ship it!
Ship It!
- Sumit Mohanty
On March 14, 2017, 10:35 p.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57625/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 10:35 p.m.)
>
>
> Review request for Ambari, Robert Levas, Sumit Mohanty, Sid Wagle, and Yusaku Sako.
>
>
> Bugs: AMBARI-20453
> https://issues.apache.org/jira/browse/AMBARI-20453
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Minor refactoring and clean up in ambari-server
>
>
> Diffs
> -----
>
> ambari-server/src/main/assemblies/server.xml 768ba68
> ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java 8d54acb
> ambari-server/src/main/package/rpm/postinstall.sh 1e8e0f0
> ambari-server/src/main/python/ambari_server/resourceFilesKeeper.py 188f3ff
> ambari-server/src/main/python/ambari_server/serverConfiguration.py 3dd165b
> ambari-server/src/main/resources/scripts/check_ambari_permissions.py PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/57625/diff/1/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>
Re: Review Request 57625: Minor refactoring and clean up in
ambari-server
Posted by Yusaku Sako <yu...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57625/#review169216
-----------------------------------------------------------
Ship it!
Ship It!
- Yusaku Sako
On March 16, 2017, 2:14 p.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57625/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 2:14 p.m.)
>
>
> Review request for Ambari, Robert Levas, Sumit Mohanty, Sid Wagle, and Yusaku Sako.
>
>
> Bugs: AMBARI-20453
> https://issues.apache.org/jira/browse/AMBARI-20453
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Minor refactoring and clean up in ambari-server
>
>
> Diffs
> -----
>
> ambari-server/src/main/assemblies/server.xml 768ba68
> ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java 8d54acb
> ambari-server/src/main/package/rpm/postinstall.sh 1e8e0f0
> ambari-server/src/main/python/ambari_server/resourceFilesKeeper.py 188f3ff
> ambari-server/src/main/python/ambari_server/serverConfiguration.py 3dd165b
> ambari-server/src/main/resources/scripts/check_ambari_permissions.py PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/57625/diff/2/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>
Re: Review Request 57625: Minor refactoring and clean up in
ambari-server
Posted by Vitalyi Brodetskyi <vb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57625/
-----------------------------------------------------------
(Updated \u0411\u0435\u0440\u0435\u0437\u0435\u043d\u044c 16, 2017, 2:14 \u043f\u0456\u0441\u043b\u044f \u043f\u043e\u043b\u0443\u0434\u043d\u044f)
Review request for Ambari, Robert Levas, Sumit Mohanty, Sid Wagle, and Yusaku Sako.
Bugs: AMBARI-20453
https://issues.apache.org/jira/browse/AMBARI-20453
Repository: ambari
Description
-------
Minor refactoring and clean up in ambari-server
Diffs (updated)
-----
ambari-server/src/main/assemblies/server.xml 768ba68
ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java 8d54acb
ambari-server/src/main/package/rpm/postinstall.sh 1e8e0f0
ambari-server/src/main/python/ambari_server/resourceFilesKeeper.py 188f3ff
ambari-server/src/main/python/ambari_server/serverConfiguration.py 3dd165b
ambari-server/src/main/resources/scripts/check_ambari_permissions.py PRE-CREATION
Diff: https://reviews.apache.org/r/57625/diff/2/
Changes: https://reviews.apache.org/r/57625/diff/1-2/
Testing
-------
mvn clean test
Thanks,
Vitalyi Brodetskyi
Re: Review Request 57625: Minor refactoring and clean up in
ambari-server
Posted by Yusaku Sako <yu...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57625/#review169072
-----------------------------------------------------------
drwxr-xr-x 755 `/var/lib/ambari-server/keys/db/newcerts'
Do you agree to change permissions for secure directories to recommended 700 [y/n] (y)?
1. We are putting quotes around the directory/file in question in a weird way. The initial quote is a backtick and the closing quote is a single quote. Let's just drop the quotes all together. I don't think the quotes add much value / clarity / readability.
2. Can we change the prompt a little bit, like this: "Fix permissions for XXX to YYY (recommended) [y/n] (y)?"
- Yusaku Sako
On March 14, 2017, 10:35 p.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57625/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 10:35 p.m.)
>
>
> Review request for Ambari, Robert Levas, Sumit Mohanty, Sid Wagle, and Yusaku Sako.
>
>
> Bugs: AMBARI-20453
> https://issues.apache.org/jira/browse/AMBARI-20453
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Minor refactoring and clean up in ambari-server
>
>
> Diffs
> -----
>
> ambari-server/src/main/assemblies/server.xml 768ba68
> ambari-server/src/main/java/org/apache/ambari/server/security/CertificateManager.java 8d54acb
> ambari-server/src/main/package/rpm/postinstall.sh 1e8e0f0
> ambari-server/src/main/python/ambari_server/resourceFilesKeeper.py 188f3ff
> ambari-server/src/main/python/ambari_server/serverConfiguration.py 3dd165b
> ambari-server/src/main/resources/scripts/check_ambari_permissions.py PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/57625/diff/1/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>