Skip to content

Enable gnmi/telemetry user authorization by config_db #21363

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.

Sign up for GitHub

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

Merged
qiluo-msft merged 19 commits into from
Feb 20, 2025
+96 −51
Merged

Enable gnmi/telemetry user authorization by config_db #21363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
9a401e5
Enable gnmi/telemetry user authorization by config_db
liuh-80 Jan 9, 2025
05f42c0
Update yang model
liuh-80 Jan 9, 2025
fa702a4
Rename `--user_auth` to `--client_auth`
liuh-80 Jan 9, 2025
1af6a5f
Add user_auth pattern to YANG models
liuh-80 Jan 9, 2025
476a569
Refactor user_auth type definition
liuh-80 Jan 9, 2025
47de0d8
Refactor user authentication handling in scripts
liuh-80 Jan 10, 2025
24bb572
Remove max-elements constraint from GNMI_CLIENT_CERT_LIST
liuh-80 Jan 10, 2025
13b292a
Refactor user authentication handling in scripts
liuh-80 Jan 10, 2025
423914b
Fix syntax error in shell scripts
liuh-80 Jan 10, 2025
8b4b884
Refactor user authentication handling in telemetry scripts
liuh-80 Jan 10, 2025
30f4d64
Add debug echo for telemetry arguments
liuh-80 Jan 13, 2025
fe4090b
Fix GNMI variable checks in scripts
liuh-80 Jan 13, 2025
5253122
Refactor GNMI client certificate handling
liuh-80 Jan 14, 2025
78ed03c
Fix typo in CRL_EXPIRE_DURATION variable check
liuh-80 Jan 15, 2025
bfa73a8
Refactor JSON field extraction in scripts
liuh-80 Feb 8, 2025
d4b3572
Rename `Extract_json_field` to `extract_field`
liuh-80 Feb 10, 2025
22256a3
Validate port value in GNMI scripts
liuh-80 Feb 12, 2025
356188f
Remove single quotes around port variable
liuh-80 Feb 14, 2025
d64c967
Remove unnecessary quotes in telemetry scripts
liuh-80 Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 41 additions & 24 deletions dockers/docker-sonic-gnmi/gnmi-native.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
EXIT_TELEMETRY_VARS_FILE_NOT_FOUND=1
INCORRECT_TELEMETRY_VALUE=2
TELEMETRY_VARS_FILE=/usr/share/sonic/templates/telemetry_vars.j2
ESCAPE_QUOTE="'\''"

extract_field() {
echo $(echo $1 | jq -r $2)
}

if [ ! -f "$TELEMETRY_VARS_FILE" ]; then
echo "Telemetry vars template file not found"
Expand All @@ -25,30 +30,29 @@ TELEMETRY_ARGS=" -logtostderr"
export CVL_SCHEMA_PATH=/usr/sbin/schema

if [ -n "$CERTS" ]; then
SERVER_CRT=$(echo $CERTS | jq -r '.server_crt')
SERVER_KEY=$(echo $CERTS | jq -r '.server_key')
SERVER_CRT=$(extract_field "$CERTS" '.server_crt')
SERVER_KEY=$(extract_field "$CERTS" '.server_key')
if [ -z $SERVER_CRT ] || [ -z $SERVER_KEY ]; then
TELEMETRY_ARGS+=" --insecure"
else
TELEMETRY_ARGS+=" --server_crt $SERVER_CRT --server_key $SERVER_KEY "
fi

CA_CRT=$(echo $CERTS | jq -r '.ca_crt')
CA_CRT=$(extract_field "$CERTS" '.ca_crt')
if [ ! -z $CA_CRT ]; then
TELEMETRY_ARGS+=" --ca_crt $CA_CRT"
fi

TELEMETRY_ARGS+=" --config_table_name GNMI_CLIENT_CERT"
elif [ -n "$X509" ]; then
SERVER_CRT=$(echo $X509 | jq -r '.server_crt')
SERVER_KEY=$(echo $X509 | jq -r '.server_key')
SERVER_CRT=$(extract_field "$X509" '.server_crt')
SERVER_KEY=$(extract_field "$X509" '.server_key')
if [ -z $SERVER_CRT ] || [ -z $SERVER_KEY ]; then
TELEMETRY_ARGS+=" --insecure"
else
TELEMETRY_ARGS+=" --server_crt $SERVER_CRT --server_key $SERVER_KEY "
fi

CA_CRT=$(echo $X509 | jq -r '.ca_crt')
CA_CRT=$(extract_field "$X509" '.ca_crt')
if [ ! -z $CA_CRT ]; then
TELEMETRY_ARGS+=" --ca_crt $CA_CRT"
fi
Expand All @@ -60,34 +64,27 @@ fi
if [ -z "$GNMI" ]; then
PORT=8080
else
PORT=$(echo $GNMI | jq -r '.port')
PORT=$(extract_field "$GNMI" '.port')
if ! [[ $PORT =~ ^[0-9]+$ ]]; then
echo "Incorrect port value ${PORT}, expecting positive integers" >&2
exit $INCORRECT_TELEMETRY_VALUE
fi
fi

TELEMETRY_ARGS+=" --port $PORT"

CLIENT_AUTH=$(echo $GNMI | jq -r '.client_auth')
CLIENT_AUTH=$(extract_field "$GNMI" '.client_auth')
if [ -z $CLIENT_AUTH ] || [ $CLIENT_AUTH == "false" ]; then
TELEMETRY_ARGS+=" --allow_no_client_auth"
fi

LOG_LEVEL=$(echo $GNMI | jq -r '.log_level')
LOG_LEVEL=$(extract_field "$GNMI" '.log_level')
if [[ $LOG_LEVEL =~ ^[0-9]+$ ]]; then
TELEMETRY_ARGS+=" -v=$LOG_LEVEL"
else
TELEMETRY_ARGS+=" -v=2"
fi

if [ -nz "$GNMI" ]; then
ENABLE_CRL=$(echo $GNMI | jq -r '.enable_crl')
if [ $ENABLE_CRL == "true" ]; then
TELEMETRY_ARGS+=" --enable_crl"
fi

CRL_EXPIRE_DURATION=$(echo $GNMI | jq -r '.crl_expire_duration')
if [ -n $CRL_EXPIRE_DURATION ]; then
TELEMETRY_ARGS+=" --crl_expire_duration $CRL_EXPIRE_DURATION"
fi
fi

# Enable ZMQ for SmartSwitch
LOCALHOST_SUBTYPE=`sonic-db-cli CONFIG_DB hget "DEVICE_METADATA|localhost" "subtype"`
if [[ x"${LOCALHOST_SUBTYPE}" == x"SmartSwitch" ]]; then
Expand All @@ -101,7 +98,7 @@ if [[ x"${MGMT_VRF_ENABLED}" == x"true" ]]; then
fi

# Server will handle threshold connections consecutively
THRESHOLD_CONNECTIONS=$(echo $GNMI | jq -r '.threshold')
THRESHOLD_CONNECTIONS=$(extract_field "$GNMI" '.threshold')
if [[ $THRESHOLD_CONNECTIONS =~ ^[0-9]+$ ]]; then
TELEMETRY_ARGS+=" --threshold $THRESHOLD_CONNECTIONS"
else
Expand All @@ -114,7 +111,7 @@ else
fi

# Close idle connections after certain duration (in seconds)
IDLE_CONN_DURATION=$(echo $GNMI | jq -r '.idle_conn_duration')
IDLE_CONN_DURATION=$(extract_field "$GNMI" '.idle_conn_duration')
if [[ $IDLE_CONN_DURATION =~ ^[0-9]+$ ]]; then
TELEMETRY_ARGS+=" --idle_conn_duration $IDLE_CONN_DURATION"
else
Expand All @@ -126,4 +123,24 @@ else
fi
fi

USER_AUTH=$(extract_field "$GNMI" '.user_auth')
if [ ! -z "$USER_AUTH" ] && [ $USER_AUTH != "null" ]; then
TELEMETRY_ARGS+=" --client_auth $USER_AUTH"

if [ $USER_AUTH == "cert" ]; then
TELEMETRY_ARGS+=" --config_table_name GNMI_CLIENT_CERT"

ENABLE_CRL=$(echo $GNMI | jq -r '.enable_crl')
if [ $ENABLE_CRL == "true" ]; then
TELEMETRY_ARGS+=" --enable_crl"
fi

CRL_EXPIRE_DURATION=$(extract_field "$GNMI" '.crl_expire_duration')
if [ ! -z "$CRL_EXPIRE_DURATION" ] && [ $CRL_EXPIRE_DURATION != "null" ]; then
TELEMETRY_ARGS+=" --crl_expire_duration $CRL_EXPIRE_DURATION"
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

$CRL_EXPIRE_DURATION

Is it possible to command inject by malicious user? #Closed

Copy link
Contributor Author

@liuh-80 liuh-80 Jan 31, 2025

Choose a reason for hiding this comment

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

It's possible but it's out of scope of this PR.
There are lots of SONiC service start with bash script and Redis config.

This is a Redis permission issue.

Copy link
Contributor Author

@liuh-80 liuh-80 Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This script start gnmi service with 'exec' command, which does not have injection issue, because it will 'replace' current bash process with telemetry process:

https://linux.die.net/man/1/bash

I verified with:

// change config_db to make /test after gnmi start
sonic-db-cli CONFIG_DB hset 'GNMI|certs' server_crt '/etc/sonic/telemetry/streamingtelemetryserver.cer && mkdir /test #'

The telemetry service will failed with a invalid command, but will not run the 'mkdir /test' part.

Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 18, 2025

Choose a reason for hiding this comment

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

suggest && -> || in test.

Copy link
Contributor Author

@liuh-80 liuh-80 Feb 19, 2025

Choose a reason for hiding this comment

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

Test with all these, not found security issue, this is because 'exec' will pass all content behind it as parameter, so && -> || will not break and run another command.

After change config DB, GNMI service can't start and the 'mkdir /test' command not execute:

root@vlab-01:/# sonic-db-cli CONFIG_DB hset 'GNMI|certs' server_crt '/etc/sonic/telemetry/streamingtelemetryserver.cer && mkdir /test #'
0
root@vlab-01:/# /usr/bin/gnmi-native.sh
/usr/bin/gnmi-native.sh: line 36: [: too many arguments
gnmi args: -logtostderr --server_crt /etc/sonic/telemetry/streamingtelemetryserver.cer && mkdir /test # --server_key '/etc/sonic/telemetry/streamingtelemetryserver.key' --ca_crt '/etc/sonic/telemetry/dsmsroot.cer' --port 50052 -v=2 --threshold 100 --idle_conn_duration 5
I0219 01:09:02.072311 878 telemetry.go:194] client_auth not provided, using defaults.
E0219 01:09:02.072380 878 telemetry.go:69] Unable to setup telemetry config due to err: port must be > 0.
root@vlab-01:/#

root@vlab-01:/# ls
bin boot dev etc home lib lib64 media mnt opt proc root run sbin sonic srv sys tmp usr var

root@vlab-01:/# sonic-db-cli CONFIG_DB hset 'GNMI|certs' server_crt '/etc/sonic/telemetry/streamingtelemetryserver.cer || mkdir /test #'
0
root@vlab-01:/# /usr/bin/gnmi-native.sh
/usr/bin/gnmi-native.sh: line 36: [: too many arguments
gnmi args: -logtostderr --server_crt /etc/sonic/telemetry/streamingtelemetryserver.cer || mkdir /test # --server_key /etc/sonic/telemetry/streamingtelemetryserver.key --ca_crt /etc/sonic/telemetry/dsmsroot.cer --port 50052 -v=2 --threshold 100 --idle_conn_duration 5
I0219 01:06:40.599396 757 telemetry.go:194] client_auth not provided, using defaults.
E0219 01:06:40.599477 757 telemetry.go:69] Unable to setup telemetry config due to err: port must be > 0.

root@vlab-01:/# ls
bin boot dev etc home lib lib64 media mnt opt proc root run sbin sonic srv sys tmp usr var

root@vlab-01:/# sonic-db-cli CONFIG_DB hset 'GNMI|certs' server_crt '/etc/sonic/telemetry/streamingtelemetryserver.cer -> mkdir /test #'
0
root@vlab-01:/# /usr/bin/gnmi-native.sh
/usr/bin/gnmi-native.sh: line 35: [: syntax error: `->' unexpected
gnmi args: -logtostderr --server_crt /etc/sonic/telemetry/streamingtelemetryserver.cer -> mkdir /test # --server_key /etc/sonic/telemetry/streamingtelemetryserver.key --ca_crt /etc/sonic/telemetry/dsmsroot.cer --port 50052 -v=2 --threshold 100 --idle_conn_duration 5
flag provided but not defined: ->
Usage of /usr/sbin/telemetry:
-allow_no_client_auth

root@vlab-01:/# ls
bin boot dev etc home lib lib64 media mnt opt proc root run sbin sonic srv sys tmp usr var

fi
fi
fi

echo "gnmi args: $TELEMETRY_ARGS"
exec /usr/sbin/telemetry ${TELEMETRY_ARGS}
67 changes: 41 additions & 26 deletions dockers/docker-sonic-telemetry/telemetry.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
EXIT_TELEMETRY_VARS_FILE_NOT_FOUND=1
INCORRECT_TELEMETRY_VALUE=2
TELEMETRY_VARS_FILE=/usr/share/sonic/templates/telemetry_vars.j2
ESCAPE_QUOTE="'\''"

extract_field() {
echo $(echo $1 | jq -r $2)
}

if [ ! -f "$TELEMETRY_VARS_FILE" ]; then
echo "Telemetry vars template file not found"
Expand All @@ -25,31 +30,28 @@ export CVL_SCHEMA_PATH=/usr/sbin/schema
export GOTRACEBACK=crash

if [ -n "$CERTS" ]; then
SERVER_CRT=$(echo $CERTS | jq -r '.server_crt')
SERVER_KEY=$(echo $CERTS | jq -r '.server_key')
SERVER_CRT=$(extract_field "$CERTS" '.server_crt')
SERVER_KEY=$(extract_field "$CERTS" '.server_key')
if [ -z $SERVER_CRT ] || [ -z $SERVER_KEY ]; then
TELEMETRY_ARGS+=" --insecure"
else
TELEMETRY_ARGS+=" --server_crt $SERVER_CRT --server_key $SERVER_KEY "
fi

CA_CRT=$(echo $CERTS | jq -r '.ca_crt')
CA_CRT=$(extract_field "$CERTS" '.ca_crt')
if [ ! -z $CA_CRT ]; then
TELEMETRY_ARGS+=" --ca_crt $CA_CRT"
fi

# Reuse GNMI_CLIENT_CERT for telemetry service
TELEMETRY_ARGS+=" --config_table_name GNMI_CLIENT_CERT"
elif [ -n "$X509" ]; then
SERVER_CRT=$(echo $X509 | jq -r '.server_crt')
SERVER_KEY=$(echo $X509 | jq -r '.server_key')
SERVER_CRT=$(extract_field "$X509" '.server_crt')
SERVER_KEY=$(extract_field "$X509" '.server_key')
if [ -z $SERVER_CRT ] || [ -z $SERVER_KEY ]; then
TELEMETRY_ARGS+=" --insecure"
else
TELEMETRY_ARGS+=" --server_crt $SERVER_CRT --server_key $SERVER_KEY "
fi

CA_CRT=$(echo $X509 | jq -r '.ca_crt')
CA_CRT=$(extract_field "$X509" '.ca_crt')
if [ ! -z $CA_CRT ]; then
TELEMETRY_ARGS+=" --ca_crt $CA_CRT"
fi
Expand All @@ -61,34 +63,26 @@ fi
if [ -z "$GNMI" ]; then
PORT=8080
else
PORT=$(echo $GNMI | jq -r '.port')
PORT=$(extract_field "$GNMI" '.port')
if ! [[ $PORT =~ ^[0-9]+$ ]]; then
echo "Incorrect port value ${PORT}, expecting positive integers" >&2
exit $INCORRECT_TELEMETRY_VALUE
fi
fi
TELEMETRY_ARGS+=" --port $PORT"

CLIENT_AUTH=$(echo $GNMI | jq -r '.client_auth')
CLIENT_AUTH=$(extract_field "$GNMI" '.client_auth')
if [ -z $CLIENT_AUTH ] || [ $CLIENT_AUTH == "false" ]; then
TELEMETRY_ARGS+=" --allow_no_client_auth"
fi

LOG_LEVEL=$(echo $GNMI | jq -r '.log_level')
LOG_LEVEL=$(extract_field "$GNMI" '.log_level')
if [[ $LOG_LEVEL =~ ^[0-9]+$ ]]; then
TELEMETRY_ARGS+=" -v=$LOG_LEVEL"
else
TELEMETRY_ARGS+=" -v=2"
fi

if [ -nz "$GNMI" ]; then
ENABLE_CRL=$(echo $GNMI | jq -r '.enable_crl')
if [ $ENABLE_CRL == "true" ]; then
TELEMETRY_ARGS+=" --enable_crl"
fi

CRL_EXPIRE_DURATION=$(echo $GNMI | jq -r '.crl_expire_duration')
if [ -n $CRL_EXPIRE_DURATION ]; then
TELEMETRY_ARGS+=" --crl_expire_duration $CRL_EXPIRE_DURATION"
fi
fi

# gNMI save-on-set behavior is disabled by default.
# Save-on-set can be turned on by setting the "TELEMETRY|gnmi|save_on_set"
# to "true".
Expand All @@ -98,7 +92,7 @@ if [ ! -z "$SAVE_ON_SET" ]; then
fi

# Server will handle threshold connections consecutively
THRESHOLD_CONNECTIONS=$(echo $GNMI | jq -r '.threshold')
THRESHOLD_CONNECTIONS=$(extract_field "$GNMI" '.threshold')
if [[ $THRESHOLD_CONNECTIONS =~ ^[0-9]+$ ]]; then
TELEMETRY_ARGS+=" --threshold $THRESHOLD_CONNECTIONS"
else
Expand All @@ -111,7 +105,7 @@ else
fi

# Close idle connections after certain duration (in seconds)
IDLE_CONN_DURATION=$(echo $GNMI | jq -r '.idle_conn_duration')
IDLE_CONN_DURATION=$(extract_field "$GNMI" '.idle_conn_duration')
if [[ $IDLE_CONN_DURATION =~ ^[0-9]+$ ]]; then
TELEMETRY_ARGS+=" --idle_conn_duration $IDLE_CONN_DURATION"
else
Expand All @@ -124,4 +118,25 @@ else
fi
TELEMETRY_ARGS+=" -gnmi_native_write=false"

USER_AUTH=$(extract_field "$GNMI" '.user_auth')
if [ ! -z "$USER_AUTH" ] && [ $USER_AUTH != "null" ]; then
TELEMETRY_ARGS+=" --client_auth $USER_AUTH"

if [ $USER_AUTH == "cert" ]; then
# Reuse GNMI_CLIENT_CERT for telemetry service
TELEMETRY_ARGS+=" --config_table_name GNMI_CLIENT_CERT"

ENABLE_CRL=$(echo $GNMI | jq -r '.enable_crl')
if [ $ENABLE_CRL == "true" ]; then
TELEMETRY_ARGS+=" --enable_crl"
fi

CRL_EXPIRE_DURATION=$(extract_field "$GNMI" '.crl_expire_duration')
if [ ! -z "$CRL_EXPIRE_DURATION" ] && [ $CRL_EXPIRE_DURATION != "null" ]; then
TELEMETRY_ARGS+=" --crl_expire_duration $CRL_EXPIRE_DURATION"
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

$CRL_EXPIRE_DURATION

the same #Closed

Copy link
Contributor Author

@liuh-80 liuh-80 Feb 8, 2025

Choose a reason for hiding this comment

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

Fixed

fi
fi
fi

echo "telemetry args: $TELEMETRY_ARGS"
exec /usr/sbin/telemetry ${TELEMETRY_ARGS}
8 changes: 7 additions & 1 deletion src/sonic-yang-models/yang-models/sonic-gnmi.yang
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,20 @@ module sonic-gnmi {
type uint32;
description "Certificate revocation list cache expire duration.";
}

leaf user_auth {
type string {
pattern 'password|jwt|cert';
}
description "GNMI service user authorization type.";
}
}
}

container GNMI_CLIENT_CERT {
description "GNMI client cert list";

list GNMI_CLIENT_CERT_LIST {
max-elements 8;
key "cert_cname";

leaf cert_cname {
Expand Down
7 changes: 7 additions & 0 deletions src/sonic-yang-models/yang-models/sonic-telemetry.yang
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,13 @@ module sonic-telemetry {
type uint32;
description "Certificate revocation list cache expire duration.";
}

leaf user_auth {
type string {
pattern 'password|jwt|cert';
}
description "Telemetry service user authorization type.";
}
}

}
Expand Down
Loading