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

SNMPv3 over DTLS #133

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

SNMPv3 over DTLS #133

wants to merge 44 commits into from

Conversation

kdurkin77
Copy link

Updated to use DTLS2.Net to allow for SNMPv3 over DTLS. I updated DTLS.Net but since they haven't been responding to pull requests or issues I had use it from my fork and called it DTLS2.Net.

TSM doesn't use the Security Parameters part of the message, so set up SecurityParameters to have a default and Header, GetRequestMessage, and MessageFactory to not require them.

Added the SecurityModel enum to allow for either TSM or USM, keeping USM default. This required new constructors in GetRequestMessage to take in the SecurityModel

Test example:

var vList = new List<Variable>() { new Variable(new ObjectIdentifier("1.3.6.1.2.1.1.3.0")) };
var receiver = new IPEndPoint(ip, port);
var auth = TsmAuthenticationProvider.Instance;
IPrivacyProvider priv = new TsmPrivacyProvider(auth);

using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser);
store.Open(OpenFlags.ReadOnly);
var myCertCollection = store.Certificates.Find(X509FindType.FindByThumbprint, "", true);

var chain = new X509Chain();
chain.Build(myCertCollection[0]);

var client = new Client(new IPEndPoint(IPAddress.Any, 0));
client.LoadX509Certificate(chain);
client.SupportedCipherSuites.Add(TCipherSuite.TLS_RSA_WITH_AES_256_CBC_SHA);

var request = new GetRequestMessage(VersionCode.V3, Messenger.NextMessageId, Messenger.NextRequestId, new OctetString(user), new OctetString(string.Empty), vList, priv, Messenger.MaxMessageSize);
var reply = request.GetSecureResponse(timeout, receiver, client);

foreach (var v in reply.Pdu().Variables)
{
	Console.WriteLine($"RESPONSE: {v}");
}

TSM doesn't use the Security Parameters part of the message, so set up SecurityParameters to have a default and Header, GetRequestMessage, and MessageFactory to not require them.

Added the SecurityModel enum to allow for either TSM or USM, keeping USM default. This required new constructors in GetRequestMessage to take in the SecurityModel

Test example:
var vList = new List<Variable>() { new Variable(new ObjectIdentifier("1.3.6.1.2.1.1.3.0")) };
var receiver = new IPEndPoint(ip, port);
var auth = TsmAuthenticationProvider.Instance;
IPrivacyProvider priv = new TsmPrivacyProvider(auth);

using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser);
store.Open(OpenFlags.ReadOnly);
var myCertCollection = store.Certificates.Find(X509FindType.FindByThumbprint, "", true);

var chain = new X509Chain();
chain.Build(myCertCollection[0]);

var client = new Client(new IPEndPoint(IPAddress.Any, 0));
client.LoadX509Certificate(chain);
client.SupportedCipherSuites.Add(TCipherSuite.TLS_RSA_WITH_AES_256_CBC_SHA);

var request = new GetRequestMessage(VersionCode.V3, Messenger.NextMessageId, Messenger.NextRequestId, new OctetString(user), new OctetString(string.Empty), vList, priv, Messenger.MaxMessageSize);
var reply = request.GetSecureResponse(timeout, receiver, client);

foreach (var v in reply.Pdu().Variables)
{
	Console.WriteLine($"RESPONSE: {v}");
}
Copy link
Collaborator

@lextm lextm left a comment

Choose a reason for hiding this comment

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

Thanks for your effort. Several parts need modification, and if possible unit test cases should also be added.

SharpSnmpLib/SharpSnmpLib.csproj Outdated Show resolved Hide resolved
@@ -104,6 +104,15 @@ public OctetString AuthenticationParameters
/// <value>The privacy parameters.</value>
public OctetString PrivacyParameters { get; private set; }


Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need an empty constructor?

Copy link
Author

Choose a reason for hiding this comment

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

This isn't a constructor. I'm not sure if you selected the wrong part of the code or if there's a problem with PrivacyParameters here?

@@ -178,6 +192,13 @@ public Sequence ToSequence()
/// <returns></returns>
public ISnmpData GetData(VersionCode version)
{
//if empty SecurityParameters, return an empty OctetString
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will try to avoid such.

Copy link
Author

Choose a reason for hiding this comment

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

With TSM the security parameters have to be empty. Attached is a screen shot of the decrypted bytes to show that the security parameters are empty (an empty OctetString)
image
image

SharpSnmpLib/Security/DTLS/SecureListenerBinding.cs Outdated Show resolved Hide resolved
SharpSnmpLib/Security/DTLS/MessageReceivedEventArgs.cs Outdated Show resolved Hide resolved
SharpSnmpLib/Messaging/SecureListener.cs Outdated Show resolved Hide resolved
SharpSnmpLib/Messaging/GetRequestMessage.cs Outdated Show resolved Hide resolved
SharpSnmpLib/Messaging/GetRequestMessage.cs Outdated Show resolved Hide resolved
@lextm lextm mentioned this pull request Dec 5, 2019
…to Samples).

Moved SecureMessageExtensions to SharpSnmpLib/Messaging and updated the namespace
@kdurkin77
Copy link
Author

I believe I have updated everything you've asked for except for the 2 I've asked for clarification on. Please let me know if there's anything further, and thank you for getting back to me so quickly

@kdurkin77
Copy link
Author

Do you have a round about ETA on when this could be released?

@lextm
Copy link
Collaborator

lextm commented Dec 10, 2019

Nope. No ETA at this moment. I am trying to set up a local lab environment to test out the changes, so it can take a relatively long period.

@kdurkin77
Copy link
Author

Understood, thank you!

@@ -82,14 +82,14 @@ public SecureDiscovery(int messageId, int requestId, int maxMessageSize)
/// <param name="responseTimeout">The time-out value, in milliseconds. The default value is 0, which indicates an infinite time-out period. Specifying -1 also indicates an infinite time-out period.</param>
/// <param name="receiver">The receiver.</param>
/// <returns></returns>
public ReportMessage GetResponse(int connectionTimeout, int responseTimeout, IPEndPoint receiver, Client client)
public async Task<ReportMessage> GetResponse(int connectionTimeout, int responseTimeout, IPEndPoint receiver, Client client)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then you should rename it to GetResponseAsync.

Copy link
Author

Choose a reason for hiding this comment

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

Will update


namespace Lextm.SharpSnmpLib.Messaging
{
public static class SecureMessageExtensions
{
public static ISnmpMessage GetSecureResponse(this ISnmpMessage request, int connectionTimeout, int responseTimeout, IPEndPoint receiver, Client client)
public static async Task<ISnmpMessage> GetSecureResponse(this ISnmpMessage request, int connectionTimeout, int responseTimeout, IPEndPoint receiver, Client client)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be changed to GetSecureResponseAsync.

Copy link
Author

Choose a reason for hiding this comment

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

Will update

@CLAassistant
Copy link

CLAassistant commented Apr 27, 2020

CLA assistant check
All committers have signed the CLA.

@lextm lextm force-pushed the master branch 6 times, most recently from af8c507 to 83a3a43 Compare December 17, 2021 17:21
@kdurkin77
Copy link
Author

Are there still plans to get this merged in at some point?

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.

3 participants