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

Remove Mutual Authentication (Fixing KUKSA Server token handling) #29

Merged
merged 1 commit into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion .github/workflows/dash.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ jobs:
> dependencies.txt

- name: Dash license check
uses: eclipse-kuksa/kuksa-actions/check-dash@2
uses: eclipse-kuksa/kuksa-actions/check-dash@4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying out the dash update as well @SebastianSchildt

with:
dashinput: ${{github.workspace}}/dependencies.txt
dashtoken: ${{ secrets.ECLIPSE_GITLAB_API_TOKEN}}
6 changes: 2 additions & 4 deletions docs/examples/threaded.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ Be also aware that this API returns JSON responses whose schema may vary from on
- `port` server/databroker port, default: 8090
- `protocol` protocol used to interact with server/databroker ("ws" or "grpc"), default: "ws"
- `insecure` whether the communication should be unencrypted or not, default: `False`
- `cacertificate` root certificate path, default: "../kuksa_certificates/CA.pem"
- `certificate` client certificate path, default: "../kuksa_certificates/Client.pem"
- `key` client private key path, default: "../kuksa_certificates/Client.key"
- `cacertificate` root certificate path, default: ""

```python
# An empty configuration dictionary will use the aforementioned default values:
Expand Down Expand Up @@ -176,5 +174,5 @@ Test Client> getValue Vehicle.Speed
"error": "timeout"
}

Test Client>
Test Client>
```
23 changes: 0 additions & 23 deletions kuksa-client/kuksa_client/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@

DEFAULT_KUKSA_ADDRESS = os.environ.get("KUKSA_ADDRESS", "grpc://127.0.0.1:55555")
DEFAULT_TOKEN_OR_TOKENFILE = os.environ.get("TOKEN_OR_TOKENFILE", None)
DEFAULT_CERTIFICATE = os.environ.get("CERTIFICATE", None)
DEFAULT_KEYFILE = os.environ.get("KEYFILE", None)
DEFAULT_CACERTIFICATE = os.environ.get("CACERTIFICATE", None)
DEFAULT_TLS_SERVER_NAME = os.environ.get("TLS_SERVER_NAME", None)

Expand Down Expand Up @@ -317,8 +315,6 @@ def __init__(
self,
server=None,
token_or_tokenfile=None,
certificate=None,
keyfile=None,
cacertificate=None,
tls_server_name=None,
):
Expand All @@ -340,8 +336,6 @@ def __init__(
self.subscribeIds = set()
self.commThread = None
self.token_or_tokenfile = token_or_tokenfile
self.certificate = certificate
self.keyfile = keyfile
self.cacertificate = cacertificate
self.tls_server_name = tls_server_name

Expand Down Expand Up @@ -612,10 +606,6 @@ def connect(self):
# Configs should only be added if they actually have a value
if self.token_or_tokenfile is not None:
config["token_or_tokenfile"] = self.token_or_tokenfile
if self.certificate is not None:
config["certificate"] = self.certificate
if self.keyfile is not None:
config["keyfile"] = self.keyfile
if self.cacertificate is not None:
config["cacertificate"] = self.cacertificate
if self.tls_server_name is not None:
Expand Down Expand Up @@ -684,17 +674,6 @@ def main():
)

# Add TLS arguments
# Note: Databroker does not yet support mutual authentication, so no need to use two first arguments
parser.add_argument(
"--certificate",
default=DEFAULT_CERTIFICATE,
help="Client cert file(.pem), only needed for mutual authentication",
)
parser.add_argument(
"--keyfile",
default=DEFAULT_KEYFILE,
help="Client private key file (.key), only needed for mutual authentication",
)
parser.add_argument(
"--cacertificate",
default=DEFAULT_CACERTIFICATE,
Expand All @@ -716,8 +695,6 @@ def main():
clientApp = TestClient(
args.server,
token_or_tokenfile=args.token_or_tokenfile,
certificate=args.certificate,
keyfile=args.keyfile,
cacertificate=args.cacertificate,
tls_server_name=args.tls_server_name,
)
Expand Down
2 changes: 0 additions & 2 deletions kuksa-client/kuksa_client/cli_backend/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ def __init__(self, config):
# If no CA Certificate is given we will use an insecure connection, requested or not
if self.cacertificate is None:
self.insecure = True
self.certificate = config.get('certificate', None)
self.keyfile = config.get('keyfile', None)
self.tls_server_name = config.get('tls_server_name', "")
self.token_or_tokenfile = config.get('token_or_tokenfile', None)

Expand Down
6 changes: 0 additions & 6 deletions kuksa-client/kuksa_client/cli_backend/grpc.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,6 @@ def __init__(self, config):
super().__init__(config)
if self.cacertificate is not None:
self.cacertificate = pathlib.Path(self.cacertificate)
if self.keyfile is not None:
self.keyfile = pathlib.Path(self.keyfile)
if self.certificate is not None:
self.certificate = pathlib.Path(self.certificate)
if self.token_or_tokenfile is not None:
if os.path.isfile(self.token_or_tokenfile):
self.token_or_tokenfile = pathlib.Path(self.token_or_tokenfile)
Expand Down Expand Up @@ -285,8 +281,6 @@ async def mainLoop(self):
self.serverIP,
self.serverPort,
root_certificates=self.cacertificate,
private_key=self.keyfile,
certificate_chain=self.certificate,
tls_server_name=self.tls_server_name,
token=self.token
) as vss_client:
Expand Down
2 changes: 0 additions & 2 deletions kuksa-client/kuksa_client/cli_backend/ws.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,6 @@ async def connect(self, _=None):
subprotocols = ["VISSv2"]
if not self.insecure:
context = ssl.create_default_context()
context.load_cert_chain(
certfile=self.certificate, keyfile=self.keyfile)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do not want to remove support for client auth then this statement must be modified by checking that both are not null before calling it

context.load_verify_locations(cafile=self.cacertificate)
# We want host name to match
# For example certificates we use subjectAltName to make it match for Server, localahost and 127.0.0.1
Expand Down
14 changes: 1 addition & 13 deletions kuksa-client/kuksa_client/grpc/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -559,8 +559,6 @@ def __init__(
port: int,
token: Optional[str] = None,
root_certificates: Optional[Path] = None,
private_key: Optional[Path] = None,
certificate_chain: Optional[Path] = None,
ensure_startup_connection: bool = True,
connected: bool = False,
tls_server_name: Optional[str] = None
Expand All @@ -569,8 +567,6 @@ def __init__(
self.authorization_header = self.get_authorization_header(token)
self.target_host = f'{host}:{port}'
self.root_certificates = root_certificates
self.private_key = private_key
self.certificate_chain = certificate_chain
self.tls_server_name = tls_server_name
self.ensure_startup_connection = ensure_startup_connection
self.connected = connected
Expand All @@ -580,15 +576,7 @@ def _load_creds(self) -> Optional[grpc.ChannelCredentials]:
if self.root_certificates:
logger.info(f"Using TLS with Root CA from {self.root_certificates}")
root_certificates = self.root_certificates.read_bytes()
if self.private_key and self.certificate_chain:
private_key = self.private_key.read_bytes()
certificate_chain = self.certificate_chain.read_bytes()
# As of today there is no option in KUKSA.val Databroker to require client authentication
logger.info("Using client private key and certificates, mutual TLS supported if supported by server")
return grpc.ssl_channel_credentials(root_certificates, private_key, certificate_chain)
else:
logger.info("No client certificates provided, mutual TLS not supported!")
return grpc.ssl_channel_credentials(root_certificates)
return grpc.ssl_channel_credentials(root_certificates)
logger.info("No Root CA present, it will not be possible to use a secure connection!")
return None

Expand Down
2 changes: 1 addition & 1 deletion kuksa-client/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ async def secure_val_server_fixture(unused_tcp_port, resources_path, val_service
(resources_path / 'test-server.pem').read_bytes(),
)],
root_certificates=(resources_path / 'test-ca.pem').read_bytes(),
require_client_auth=True,
require_client_auth=False,
))
await server.start()
try:
Expand Down
27 changes: 0 additions & 27 deletions kuksa-client/tests/resources/test-client.key

This file was deleted.

22 changes: 0 additions & 22 deletions kuksa-client/tests/resources/test-client.pem

This file was deleted.

2 changes: 0 additions & 2 deletions kuksa-client/tests/test_grpc.py
Original file line number Diff line number Diff line change
Expand Up @@ -406,8 +406,6 @@ async def test_secure_connection(self, unused_tcp_port, resources_path, val_serv
name='test_server', version='1.2.3')
async with VSSClient('localhost', unused_tcp_port,
root_certificates=resources_path / 'test-ca.pem',
private_key=resources_path / 'test-client.key',
certificate_chain=resources_path / 'test-client.pem',
ensure_startup_connection=True
):
assert val_servicer.GetServerInfo.call_count == 1
Expand Down