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

Packaging clean-up + SSL warning in http_check #778

Merged
merged 18 commits into from
Jan 7, 2014

Conversation

alq666
Copy link
Member

@alq666 alq666 commented Dec 29, 2013

Fixes #768, fixes #769 and fixes #770.

  1. dd-agent user does not need a shell anymore.
  2. dd-agent user is deleted when the package is uninstalled.
  3. SSL certificate validation for http_check is left to False (in which case it issues a warning). It can be set to True in `http_check; it acts as a control. The default is to keep certificate validation disabled to remain compatible with previous versions.
  4. Creates a Vagrant testbed (debian + redhat) that I used for validation.

Tested on Debian 7.1 and CentOS 6.5.

@ghost ghost assigned remh Dec 29, 2013
@alq666
Copy link
Member Author

alq666 commented Dec 29, 2013

@amitto this pertains to security tweaks to our agent per our conversation in Burlingame.

@alq666
Copy link
Member Author

alq666 commented Dec 30, 2013

Also fixes #771

@@ -0,0 +1,48 @@
# -*- mode: ruby -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove that file from your PR ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can but bear in mind that this is caused by vagrant init.

@@ -3,7 +3,7 @@

getent group dd-agent >/dev/null || groupadd -r dd-agent
getent passwd dd-agent >/dev/null || \
useradd -r -M -g dd-agent -d /tmp -s /bin/sh \
useradd -r -M -g dd-agent -d /tmp -s /bin/false \
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way,
Why are we creating a user with a home directory on centos/rhel and not on debian/ubuntu ?

Can't we use a system account instead without home directory ?
http://unix.stackexchange.com/questions/22275/how-to-create-an-unprivileged-user-in-centos

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clearer it would mean creating the user using:

useradd -r -M -g dd-agent -d /dev/null -s /bin/false \

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. The least amount of changes we apply, the easier it is.
On Dec 31, 2013 10:48 AM, "Remi Hakim" notifications@github.com wrote:

In packaging/datadog-agent-rpm/preinst:

@@ -3,7 +3,7 @@

getent group dd-agent >/dev/null || groupadd -r dd-agent
getent passwd dd-agent >/dev/null || \

  • useradd -r -M -g dd-agent -d /tmp -s /bin/sh \
  • useradd -r -M -g dd-agent -d /tmp -s /bin/false \

To be clearer it would mean creating the user using:

useradd -r -M -g dd-agent -d /dev/null -s /bin/false \


Reply to this email directly or view it on GitHubhttps://github.com//pull/778/files#r8599675
.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not even sure we need -d /dev/null as according to the documentation

-r creates a system account with a UID less than 500 and without a home directory

http://www.centos.org/docs/5/html/5.2/Deployment_Guide/s2-users-add.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out -r still populates the home directory field in /etc/passwd so we
can set it to /usr/share/datadog/agent.

On Tue, Dec 31, 2013 at 11:16 AM, Remi Hakim notifications@github.comwrote:

In packaging/datadog-agent-rpm/preinst:

@@ -3,7 +3,7 @@

getent group dd-agent >/dev/null || groupadd -r dd-agent
getent passwd dd-agent >/dev/null || \

  • useradd -r -M -g dd-agent -d /tmp -s /bin/sh \
  • useradd -r -M -g dd-agent -d /tmp -s /bin/false \

I'm not even sure we need -d /dev/null as according to the documentation

-r creates a system account with a UID less than 500 and without a home
directory

http://www.centos.org/docs/5/html/5.2/Deployment_Guide/s2-users-add.html


Reply to this email directly or view it on GitHubhttps://github.com//pull/778/files#r8599975
.

@remh I have checked and we do not depend on the shell for the agent
proper. `test_ganglia.py` is run by travis or jenkins, while the
`boto.manage` stuff is a layer of server management built on top of
boto. Fixes #769.
@alq666
Copy link
Member Author

alq666 commented Jan 5, 2014

@remh should be good for final review now.

@@ -5,7 +5,8 @@ set -e
case "$1" in
configure)
update-rc.d datadog-agent defaults
adduser --system dd-agent --shell /bin/sh --no-create-home --quiet
adduser --system dd-agent --disabled-login --shell /bin/false --no-create-home --quiet
usermod -d /usr/share/datadog/agent dd-agent
Copy link
Member Author

Choose a reason for hiding this comment

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

Point to an existing directory, but disable logins.

@remh
Copy link
Contributor

remh commented Jan 7, 2014

@alq666 Looks fine to me.
Feel free to merge to master, it will be part of 4.1

alq666 added a commit that referenced this pull request Jan 7, 2014
Packaging clean-up + SSL warning in http_check
@alq666 alq666 merged commit 31a7cdb into master Jan 7, 2014
@alq666 alq666 deleted the pre-post-install-cleanup branch January 7, 2014 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants