Skip to content

fix: being able to use empty DNS_ADDRS with docker image - #2866

Merged
madeye merged 1 commit into
shadowsocks:masterfrom
alexandre-abrioux:docker-empty-dns
Apr 11, 2022
Merged

madeye merged 1 commit into
shadowsocks:masterfrom
alexandre-abrioux:docker-empty-dns

Conversation

@alexandre-abrioux

@alexandre-abrioux alexandre-abrioux commented Dec 8, 2021 •

Copy link
Copy Markdown
Contributor

Hi! First of all, thank you for maintaining this amazing tool.

Issue

Overriding the image's default $DNS_ADDRS with an empty string still adds the -d option to the command.

Solution

Make sure that we only add the -d option if $DNS_ADDRS is defined and not empty.

@predatorray

Copy link
Copy Markdown

I think the problem we have here is that, the arguments passed to ss-server are not double-quoted. This will finally cause the issue mentioned above. By provided an empty DNS_ADDRS. The entrypoint.sh will execute the command ss-server -s 0.0.0.0 -p 8388 -k PASSWORD -m aes-256-gcm -t 300 -d -u.

Another issue we would possibly face is the environment variable can somehow be code-injected. For example, executing the command below.

docker run -it --rm \
    -e SERVER_ADDR='0.0.0.0 -p 8080' \
    shadowsocks/shadowsocks-libev:v3.3.5

The main process would be:

ss-server
-s
0.0.0.0
-p
8080
-p
8388
-k
PASSWORD
-m
aes-256-gcm
-t
300
-d
8.8.8.8,8.8.4.4
-u

A quick fix would be making every argument be double-quoted. But I'm not sure whether passing empty string to -e results the same as leaving it absent.

@alexandre-abrioux

alexandre-abrioux commented Dec 17, 2021 •

Copy link
Copy Markdown
Contributor Author

Just a precision for the reviewers, the goal of this PR is to be able to use the container's default DNS server, meaning the one defined in /etc/resolv.conf. See the documentation of ss-server:

-d <addr>
Setup name servers for internal DNS resolver (libc-ares). The default server is fetched from /etc/resolv.conf.

Like @predatorray said, it might work with just using -d "" instead of completely removing the argument, but I haven't testing it either. I'm not sure if it is the same behavior (see the code below). Let me know the preferred way of doing it 🙂
https://github.com/shadowsocks/shadowsocks-libev/blob/89b5f987d6a5329de9713704615581d363f0cfed/src/server.c#L1668-L1670

@madeye
madeye merged commit e21f824 into shadowsocks:master Apr 11, 2022
@alexandre-abrioux
alexandre-abrioux deleted the docker-empty-dns branch April 11, 2022 15:09
@predatorray

Copy link
Copy Markdown

@madeye I am wondering when this change will be released as a stable image so that my next helm chart release can use?

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