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

Dockerfile that builds with poetry #493

Merged
merged 7 commits into from
Feb 10, 2022
Merged

Dockerfile that builds with poetry #493

merged 7 commits into from
Feb 10, 2022

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Feb 9, 2022

To test:

DOCKER_BUILDKIT=1 docker build . -t sydent && docker run -p 8090:8090 sydent

I've seen this print sydent's startup lines and persist data to the /data volume. I could make a GET request to localhost:8090/ and got a 404 error page for my troubles, together with an entry in Sydent's log. I haven't tested this any more thoroughly than that.

To inspect the container while it's running, get the container id with
docker ps and then:

$ docker exec -it 001f9bfc6a54 bash
sydent@001f9bfc6a54:/home/sydent$ ls
src  venv
sydent@001f9bfc6a54:/home/sydent$ ls src
README.rst  poetry.lock  pyproject.toml  requirements.txt  res scripts  sydent
sydent@001f9bfc6a54:/home/sydent$ ls venv
bin  lib  pyvenv.cfg

Pull Request Checklist

  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fix a bug that prevented receiving messages from other servers." instead of "Move X method from EventStore to EventWorkerStore.".
    • Include 'Contributed by Your Name.' or 'Contributed by @your-github-username.' — unless you would prefer not to be credited in the changelog.
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off

To test:

```
DOCKER_BUILDKIT=1 docker build . -t sydent && docker run sydent
```

To inspect the container while it's running, get the container id with
`docker ps` and then:

```
$ docker exec -it 001f9bfc6a54 bash
sydent@001f9bfc6a54:/home/sydent$ ls
src  venv
sydent@001f9bfc6a54:/home/sydent$ ls src
README.rst  poetry.lock  pyproject.toml  requirements.txt  res scripts  sydent
sydent@001f9bfc6a54:/home/sydent$ ls venv
bin  lib  pyvenv.cfg
```
@DMRobertson DMRobertson requested a review from a team as a code owner February 9, 2022 17:49
Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Seems good :)

Dockerfile Outdated Show resolved Hide resolved
@@ -39,15 +47,16 @@ RUN addgroup --system --gid 993 sydent \
&& mkdir /data \
Copy link
Contributor

Choose a reason for hiding this comment

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

I realise this isn't yours, but on the line above, why are we creating a password for the sydent user?
It originally has a disabled password. There's no reason to set a static password ... is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered about this too. I removed this from the builder step (see line -13).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to have been added in #290; wasn't present in #80. No clues in either PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I reckon we just remove it here as well. A password that is the same for every Sydent container doesn't seem to afford any security anyway.
I also notice --disabled-login is an option rather than --disabled-password

Excerpt from /etc/shadow:

dpass:*:19033:0:99999:7:::
dlogin:!:19033:0:99999:7:::

If the password field contains some string that is not a valid result of crypt(3), for instance ! or *, the user will not be able to use a unix password to log in (but the user may log in the system by other means).

man shadow

--disabled-login sounds like it (!) will also prevent SSH login using authorised keys. I guess we're not running an SSH daemon anyway, so it probably doesn't matter which one we use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A password that is the same for every Sydent container doesn't seem to afford any security anyway.

The password comes from /dev/random so I would expect it to be different every time you run build this container. I still think it's odd though.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if it gets uploaded to Docker Hub or something, everyone can then pull that and even crack the password hash if they're so keen. I'd pull it out — the password isn't even being given to anyone so I can't see how it's useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see---good point. Yes, let's excise it

David Robertson and others added 4 commits February 10, 2022 12:00
Co-authored-by: reivilibre <oliverw@matrix.org>
Co-authored-by: Shay <hillerys@element.io>
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

thanks!

@DMRobertson DMRobertson merged commit fa4ba7a into main Feb 10, 2022
@DMRobertson DMRobertson deleted the dmr/docker branch February 10, 2022 14:57
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.

3 participants