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.

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

Jump to bottom

add optional SSL support #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add optional SSL support #46

wants to merge 1 commit into from

Conversation

fsagbuya
Copy link
Contributor

@fsagbuya fsagbuya commented Oct 2, 2024 •
edited
Loading

Description

Add optional SSL/TLS support with mutual authentication. SSL is enabled when certificate files are provided.

  • Add SSL context creation in AsyncioServer requiring both server and client certificates
  • Update Client, AsyncioClient, and BestEffortClient classes to support mutual SSL authentication
  • Modify simple_server_loop to handle certificate verification
  • Add SSL parameter support to sipyco_rpctool (--cert, --key, --cafile)
  • Add documentation for SSL setup and usage including certificate generation

Related issue:

m-labs/artiq#2577

@sbourdeauducq
Copy link
Member

Needs documentation (docstring update).

@sbourdeauducq
Copy link
Member

Many things missing, e.g.

  • support in rpctool
  • support in other sipyco protocols: sync_struct, logging, broadcast
  • documentation about how to generate certificates

@sbourdeauducq
Copy link
Member

You could do the protocols one by one if you prefer, and then we merge those PRs to a separate branch first.

@fsagbuya fsagbuya marked this pull request as draft October 9, 2024 06:56
@fsagbuya fsagbuya force-pushed the ssl-support branch 2 times, most recently from 98e30d6 to 2bdae90 Compare October 9, 2024 07:58
@fsagbuya fsagbuya marked this pull request as ready for review October 9, 2024 07:58
@fsagbuya fsagbuya mentioned this pull request Oct 11, 2024
Open
@sbourdeauducq
Copy link
Member

This doesn't authenticate the client, does it?
The main point of having SSL is preventing unwanted users from connecting to sipyco servers.

@sbourdeauducq
Copy link
Member

sbourdeauducq commented Oct 21, 2024 •
edited
Loading

I think you can add a client certificate (with different SSL API calls), or just a password.

@fsagbuya fsagbuya marked this pull request as draft October 24, 2024 09:34
sbourdeauducq
sbourdeauducq reviewed Nov 7, 2024
View reviewed changes
doc/index.rst Outdated Show resolved Hide resolved
@fsagbuya fsagbuya marked this pull request as ready for review November 7, 2024 03:40
sbourdeauducq
sbourdeauducq reviewed Nov 7, 2024
View reviewed changes
doc/index.rst Outdated Show resolved Hide resolved
sbourdeauducq
sbourdeauducq reviewed Nov 7, 2024
View reviewed changes
doc/index.rst Outdated Show resolved Hide resolved
@fsagbuya fsagbuya force-pushed the ssl-support branch 2 times, most recently from 12ac4f4 to 2a7f45f Compare November 8, 2024 06:54
sbourdeauducq
sbourdeauducq reviewed Nov 16, 2024
View reviewed changes
doc/index.rst Outdated Show resolved Hide resolved
sbourdeauducq
sbourdeauducq reviewed Nov 16, 2024
View reviewed changes
doc/index.rst Outdated Show resolved Hide resolved
sbourdeauducq
sbourdeauducq reviewed Nov 16, 2024
View reviewed changes
doc/index.rst Outdated Show resolved Hide resolved
@fsagbuya fsagbuya force-pushed the ssl-support branch 2 times, most recently from be0b423 to 717b7ca Compare November 26, 2024 08:50
@fsagbuya fsagbuya mentioned this pull request Dec 3, 2024
Draft
@fsagbuya fsagbuya force-pushed the ssl-support branch 2 times, most recently from df2093b to 58e7e1d Compare December 16, 2024 09:44
sbourdeauducq
sbourdeauducq reviewed Dec 17, 2024
View reviewed changes
sipyco/ssl_tools.py
context.load_verify_locations(peerfile)
else:
context = ssl.create_default_context(cafile=peerfile)
context.check_hostname = False
Copy link
Member

Choose a reason for hiding this comment

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

Is this still actually checking the server certificate? Did you test and how?

Copy link
Contributor Author

@fsagbuya fsagbuya Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

upon testing again.. it is not. A fix is having the client context as PROTOCOL_TLS_CLIENT and have load_verify_locations. Will add this to the commit.

Copy link
Member

Choose a reason for hiding this comment

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

Add some tests as well with failing cert validations.

sbourdeauducq
sbourdeauducq reviewed Dec 17, 2024
View reviewed changes
sipyco/ssl_tools.py Outdated Show resolved Hide resolved
sbourdeauducq
sbourdeauducq reviewed Dec 17, 2024
View reviewed changes
sipyco/pc_rpc.py Outdated Show resolved Hide resolved
@fsagbuya
add optional ssl support
c0aed03 
Signed-off-by: Florian Agbuya <fa@m-labs.ph>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbourdeauducq sbourdeauducq sbourdeauducq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fsagbuya @sbourdeauducq