Skip to content

[CLI] Require explicit CA for remote Python REST connections #166

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 3 commits into from
Sep 8, 2026
+213 −10
Merged

[CLI] Require explicit CA for remote Python REST connections #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73b6119
[CLI] Verify remote REST server certificates
ashutosh-agrawal Sep 1, 2026
65becf2
[CLI] Recognize all loopback IP addresses
ashutosh-agrawal Sep 1, 2026
f0e11c8
[CLI] Require explicit CA for remote REST servers
ashutosh-agrawal Sep 5, 2026
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
4 changes: 4 additions & 0 deletions CLI/actioner/README_cli_client.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ import cli_client as cc
api = cc.ApiClient()
```

The client connects to `https://localhost` by default. To connect to a non-loopback REST server, set
`REST_API_ROOT` to the server URL and `REST_API_CA_CERT` to a readable PEM CA certificate file. The
client does not use the system CA store for non-loopback endpoints.

Create a path object for target REST resource. It accepts parameterized path template and parameter
values. Path template is similar to the template used by swagger. Parameter values will be URL-encoded
and substituted in the template to get REST resource path.
Expand Down
53 changes: 43 additions & 10 deletions CLI/actioner/cli_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,34 @@
# #
################################################################################

import ipaddress
import os
import json
import urllib3
import warnings
import requests
from requests.structures import CaseInsensitiveDict
from six.moves.urllib.parse import quote
from six.moves.urllib.parse import quote, urlparse
from urllib3.exceptions import InsecureRequestWarning
from collections import OrderedDict
from cli_log import log_info, log_warning

urllib3.disable_warnings()

REST_API_CA_CERT = 'REST_API_CA_CERT'


def _is_loopback_endpoint(url):
try:
hostname = urlparse(url).hostname
except ValueError:
return False
if hostname is None:
return False
if hostname.lower() == 'localhost':
return True
try:
return ipaddress.ip_address(hostname).is_loopback
except ValueError:
return False


class ApiClient(object):
Expand All @@ -51,14 +69,29 @@ def request(self, method, path, data=None, headers={}, query=None, response_type
req_headers['Content-Type'] = 'application/yang-data+json'
body = json.dumps(data)

verify = False
if not _is_loopback_endpoint(url):
verify = os.getenv(REST_API_CA_CERT)
if not verify:
msg = '%Error: REST_API_CA_CERT must be set for a remote Management REST Server'
log_info("cli_client certificate configuration error: {}", msg)
return ApiClient.__new_error_response(msg)
if not os.path.isfile(verify) or not os.access(verify, os.R_OK):
msg = '%Error: REST_API_CA_CERT must identify a readable CA certificate file'
log_info("cli_client certificate configuration error: {}", msg)
return ApiClient.__new_error_response(msg)

try:
r = ApiClient.__session.request(
method,
url,
headers=req_headers,
data=body,
params=query,
verify=False)
with warnings.catch_warnings():
if not verify:
warnings.simplefilter('ignore', InsecureRequestWarning)
r = ApiClient.__session.request(
method,
url,
headers=req_headers,
data=body,
params=query,
verify=verify)

@qiluo-msft qiluo-msft Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The fix correctly moves away from verify=False for remote endpoints, but using verify=True
relies on whatever CA certificates happen to be present in the container's system store.
For a managed SONiC deployment this is fragile — the container may not carry the right
enterprise CA, and there's no explicit operator control over which CA is trusted.

Suggest adding a --rest-ca-cert CLI option (or a counterpart in the existing REST
client config) that accepts a PEM-format CA file, and using that path as the verify=
argument for remote requests:

verify = ca_cert_path if not _is_loopback_endpoint(url) else False

where ca_cert_path is the value of --rest-ca-cert. If a remote endpoint is configured
and no --rest-ca-cert is supplied, the client should fail with a clear error rather than
falling back to the system store.

This gives operators an explicit, visible CA provisioning step instead of implicit
system-bundle trust.

@ashutosh-agrawal ashutosh-agrawal Sep 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@qiluo-msft Thanks. After looking further, I agree that falling back to the general system trust store does not provide REST-specific control over the trusted CA.

I propose splitting the work into focused changes. In this PR, I will update the Python actioner client to require an explicitly configured CA file for non-loopback REST_API_ROOT values, without falling back to the system trust store.

The CLI also has a C++ libcurl REST client used for authentication and token refresh. I will submit a follow-up PR applying the same explicit-CA behavior there. The operator-facing --rest-ca-cert option, including host-to-container certificate provisioning, can then be added separately in sonic-buildimage.

Would this staged approach work for you?

@ashutosh-agrawal ashutosh-agrawal Sep 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated in f0e11c8. The Python actioner now requires REST_API_CA_CERT for non-loopback endpoints and passes that PEM path directly to Requests. It does not fall back to REQUESTS_CA_BUNDLE or the system CA store, and missing or unreadable configuration returns a clear error before making the request.

I added focused unit coverage and repeated the VS validation. The configured explicit CA succeeded, while the Requests bundle fallback and an invalid explicit path were rejected. The C++ client and operator-facing CLI plumbing remain scoped to the follow-up changes described above.


return Response(r, response_type)

Expand Down
166 changes: 166 additions & 0 deletions CLI/tests/test_cli_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
import os
import sys
import unittest
import warnings
from unittest import mock

import requests
from urllib3.exceptions import InsecureRequestWarning


ACTIONER_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', 'actioner'))
sys.path.insert(0, ACTIONER_DIR)

import cli_client


def _response():
response = requests.Response()
response.status_code = 200
response._content = b''
return response


class ApiClientCertificateVerificationTest(unittest.TestCase):

def test_loopback_endpoint_detection(self):
loopback_urls = (
'https://localhost',
'https://localhost:443',
'https://127.0.0.1',
'https://127.0.0.1:8443',
'https://127.0.0.2',
'https://127.255.255.254',
'https://[::1]',
'https://[::1]:8443',
)
remote_urls = (
'https://rest.example.com',
'https://localhost.example.com',
'https://192.0.2.1',
'https://[::1',
)

for url in loopback_urls:
with self.subTest(url=url):
self.assertTrue(cli_client._is_loopback_endpoint(url))

for url in remote_urls:
with self.subTest(url=url):
self.assertFalse(cli_client._is_loopback_endpoint(url))

def test_loopback_request_skips_certificate_verification(self):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__api_root',
'https://127.0.0.1'):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__session') as session:
session.request.return_value = _response()

cli_client.ApiClient().get('/restconf/data')

self.assertFalse(session.request.call_args.kwargs['verify'])

def test_remote_request_uses_configured_ca_certificate(self):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__api_root',
'https://rest.example.com'):
with mock.patch.dict(
os.environ,
{cli_client.REST_API_CA_CERT: '/etc/sonic/rest-ca.pem'}):
with mock.patch('cli_client.os.path.isfile', return_value=True):
with mock.patch('cli_client.os.access', return_value=True):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__session') as session:
session.request.return_value = _response()

cli_client.ApiClient().get('/restconf/data')

self.assertEqual(
'/etc/sonic/rest-ca.pem',
session.request.call_args.kwargs['verify'])

def test_remote_request_requires_ca_certificate(self):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__api_root',
'https://rest.example.com'):
with mock.patch.dict(os.environ, {}, clear=True):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__session') as session:

response = cli_client.ApiClient().get('/restconf/data')

session.request.assert_not_called()
self.assertEqual(
'%Error: REST_API_CA_CERT must be set for a remote Management REST Server',
response.content['ietf-restconf:errors']['error'][0]['error-message'])

def test_remote_request_does_not_use_requests_ca_bundle(self):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__api_root',
'https://rest.example.com'):
with mock.patch.dict(
os.environ,
{'REQUESTS_CA_BUNDLE': '/etc/ssl/certs/ca-certificates.crt'},
clear=True):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__session') as session:

response = cli_client.ApiClient().get('/restconf/data')

session.request.assert_not_called()
self.assertEqual(
'%Error: REST_API_CA_CERT must be set for a remote Management REST Server',
response.content['ietf-restconf:errors']['error'][0]['error-message'])

def test_remote_request_rejects_unreadable_ca_certificate(self):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__api_root',
'https://rest.example.com'):
with mock.patch.dict(
os.environ,
{cli_client.REST_API_CA_CERT: '/etc/sonic/missing.pem'}):
with mock.patch('cli_client.os.path.isfile', return_value=False):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__session') as session:

response = cli_client.ApiClient().get('/restconf/data')

session.request.assert_not_called()
self.assertEqual(
'%Error: REST_API_CA_CERT must identify a readable CA certificate file',
response.content['ietf-restconf:errors']['error'][0]['error-message'])

def test_loopback_warning_suppression_is_scoped_to_request(self):
def request_with_warning(*args, **kwargs):
warnings.warn('loopback request', InsecureRequestWarning)
return _response()

with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__api_root',
'https://localhost'):
with mock.patch.object(
cli_client.ApiClient,
'_ApiClient__session') as session:
session.request.side_effect = request_with_warning

with warnings.catch_warnings():
warnings.simplefilter('error', InsecureRequestWarning)
cli_client.ApiClient().get('/restconf/data')
with self.assertRaises(InsecureRequestWarning):
warnings.warn('outside request', InsecureRequestWarning)


if __name__ == '__main__':
unittest.main()
Loading