Skip to content

feat(nrdot-mssql): add debian.yml/rhel.yml recipes for Linux SQL Server monitoring - #1412

Open
RamanaReddy8801 wants to merge 26 commits into
mainfrom
mssql-otel-linux-cli
Open

feat(nrdot-mssql): add debian.yml/rhel.yml recipes for Linux SQL Server monitoring#1412
RamanaReddy8801 wants to merge 26 commits into
mainfrom
mssql-otel-linux-cli

Conversation

@RamanaReddy8801

@RamanaReddy8801 RamanaReddy8801 commented Jul 28, 2026

Copy link
Copy Markdown

Jira Ticket: https://new-relic.atlassian.net/browse/NR-595360

Summary

I’ve attached the test [documentation] covering several key integration scenarios for your review.

@Nandu-pns

Nandu-pns commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

It looks like SA_PASSWORD and a few other vars are getting inserted straight into the script text using {{.NR_CLI_MSSQL_SA_PASSWORD}}, instead of being read as an environment variable when the script runs. I think this is bit risky: if the password has characters like `, $(...), or " in it, they get treated as shell code, not just as password text — that's an injection risk. It also means the actual secret is written out in the generated script instead of staying in the environment.

The redis recipe already does this the safer way — REDIS_PASSWORD="${NR_CLI_REDIS_PASSWORD:-}" — it just reads the value from the environment at runtime. Could we update debian.yml and rhel.yml to do the same for SA_PASSWORD, SERVER, PORT, and LOGIN_NAME? Should be a simple swap with no behavior change. (Same issue is in the Windows recipe too, just worth fixing there separately.)

@Nandu-pns

Copy link
Copy Markdown
Contributor

One more thing — in configure_database_user, there's an if/else meant to let someone pass in a custom password for the monitoring login instead of a random one:

if [ -z "{{.NR_CLI_MSSQL_LOGIN_PASSWORD}}" ]; then
  NR_PASSWORD=$(tr -dc 'A-Za-z0-9' < /dev/urandom | head -c 24)
else
  NR_PASSWORD="{{.NR_CLI_MSSQL_LOGIN_PASSWORD}}"
fi

NR_CLI_MSSQL_LOGIN_PASSWORD is never added to inputVars, so it always resolves to an empty string — which means the else branch (the custom password) can never actually run. Every install just takes the if branch and generates a random password, so the else here is dead code.

Not a security issue, just dead code right now. The rabbitmq recipe shows how to wire this up properly if a custom password is actually wanted:

inputVars:
  - name: NR_CLI_RABBITMQ_PASSWORD
    prompt: "RabbitMQ password: "
    secret: true

It's declared in inputVars with secret: true, so the CLI prompts for it and the value is real by the time the script runs.

If a custom password isn't actually needed, simplest is to just drop the if/else and always generate the random one — up to you which way you'd rather go.

Comment thread recipes/newrelic/infrastructure/nrdot/mssql-otel/debian.yml
printf '%s' "$NR_PASSWORD" > "$PW_FILE"
chmod 600 "$PW_FILE"

cat > "$SQL_FILE" << EOF

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar issue as the config file, but on SQL_FILE="/tmp/nr-mssql-grant.sql" (same in both debian.yml and rhel.yml): it's written with the new monitoring password in plain text but never gets chmod 600 like PW_FILE does — so it's readable by any user on the box. Worse, if sqlcmd fails, the script exits before rm -f "$SQL_FILE" ever runs, so the file (with the password) is left behind permanently.

Could you add chmod 600 "$SQL_FILE" right after it's written, and clean up both temp files even on failure (e.g. a trap ... EXIT)? Or use mktemp instead of a fixed name — other recipes here (agent-control, PHP agent) already do that, and it gets you 600 perms + an unguessable filename in one step.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in both debian.yml and rhel.yml. Went with mktemp for SQL_FILE (unguessable name + 600 perms in one step, as you suggested), plus a trap cleanup EXIT that removes it on every exit path — including a failed sqlcmd run.

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.

2 participants