You are viewing a plain text version of this content. The canonical link for it is here.

Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/06/23 21:34:13 UTC

[GitHub] [activemq-nms-openwire] brudo opened a new pull request, #23: Updates to help move forward NMS openwire 2.0.0 release

brudo opened a new pull request, #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23

I changed the default SslContext from "Tls" to "Default", which I believe will defer to the OS. I believe this is the correct behavior for a library. Note that I did not test the above change yet, so initially I am creating this as a draft pull request.

_Under .NET Framework, depending on the OS version and how it was initially configured, it may be necessary to add/change the registry setting SchUseStrongCrypto to 1 for it to use secure protocols. I don't know if there is an equivalent setting required for .NET core 3.1 or .NET 6._

Other changes to remove most warnings when rebuilding all:
- updated various tests where there was a warning about async without await, to await Task.CompletedTask.
- removed a few variables that were not referenced (an Exception, and a timeout) to prevent a compile time warning

This leaves only one warning, for use of the deprecated iconUrl as opposed to icon, in the nuspec. There were instructions for how to address that, but I left it alone, as I was not sure if it needed to stay that way for backward compatibility.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] brudo commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

brudo commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1169116217

   I can take on the work to fail fast on invalid SslProtocol value (rather than failing late, as in the original code, or not failing at all even when explicitly passed an invalid value, as I have it right now) and will include it in this PR shortly.
   
   I notice that SslContext is a private class, and that the SslContext(string) constructor is only called from the default SslContext() constructor, and there is no exposed property of type SslContext; only the string property SslProtocol. In light of this, I don't think the SslContext class needs to be touched. Rather, the validation belongs in the public SslProtocol setters in SslTransportFactory and also in SslTransport (where SslProtocol could be overridden after creating an SslTransport and before calling Start). It would be the same validation in both the factory and the transport.
   
   In the places where the sslProtocol _field_ is set from SslContext.GetCurrent().SslProtocol, the validation would not be run repeatedly, since that bypasses the public property. However, I am also changing the sslProtocol field from `private` to `internal` in SslTransport, so that SslTransportFactory can assign directly in DoCreateTransport, without having it revalidated (really a micro-optimization but trivial).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] brudo commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

brudo commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1166056469

   Reviewing the change in PR #21, I had not changed the fallback return value in GetAllowedProtocol from Default to None; I only had the change to the default constructor. In case we are squashing and merging this PR instead, I made a similar, but not identical change here for consideration.
   
   One functional difference between the two implementations - I used Enum.TryParse instead of checking the string value before calling Parse. That way it will fall back to None (0) and not raise an exception even if an invalid string (one not found in the enum) was used in the SslContext.
   
   I think this is an improvement; others may disagree... To my way of thinking, if we want to raise an exception for invalid values, it would be better to have it validate the input in the SslContext(string) constructor instead of leaving it to be thrown later, when GetAllowedProtocol is called from CreateSocketStream.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] brudo commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

brudo commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1169395622

   (Compromised test broker has been re-secured with a new password; palm to forehead self-flagellation maneuver completed.)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] michaelpearce-gain commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

michaelpearce-gain commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1168251095

   @brudo happy to have it all in this PR if youre sorting. Re commit format thats correct its basically prefix with valid jira id.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] brudo commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

brudo commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1169374202

   I squashed it down to two commits, one for the changes related to the SSL protocol, and one for the other changes to address some warnings. I hope this will fit the requirements for merging the PR.
   
   (p.s. I had accidentally included a one-off change to one of the existing tests in my final commit before the squash, where I was testing the SSL changes against a secure broker, and hardcoded the URL and credentials. This now shows up as the only change if you compare before and after the force-push; guess I'd better change the password for that test broker now!)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] michaelpearce-gain commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

michaelpearce-gain commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1168252436

   Enum sounds sensible to me, I think you have a valid argument, its def better to fail fast if incorrect config rather than later


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] brudo commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

brudo commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1165794090

   Regarding the last commit: After testing against a broker configured to only allow TLS 1.1 or 1.2, and finding that it wasn't working, I looked closer at the SslProtocols enum and realized the value "Default" doesn't defer to the OS; the bits add up to use either SSL 3.0 or TLS 1.0 - a very old default. After changing it to "None", which has a value of 0 in the enum, it seems to defer properly to the OS (or at least negotiates successfully to use a protocol that is supported by the broker).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] michaelandrepearce merged pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

michaelandrepearce merged PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] michaelpearce-gain commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

michaelpearce-gain commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1165845782

   Im happy to merge either this PR or the other. If this PR is to be taken, its the same as the other that commit needs to be squashed and also and commit message to be in correct format.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] tabish121 commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

tabish121 commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1165799423

   There's already a decent amount of discussion on this topic in PR #21 
   
   The MS [documentation](https://docs.microsoft.com/en-us/dotnet/api/system.security.authentication.sslprotocols?view=net-6.0) is pretty clear that the default should be 'none' these days
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] michaelandrepearce commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

michaelandrepearce commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1169685854

   @brudo looks good to me


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] michaelandrepearce commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

michaelandrepearce commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1169690895

   i fixed missing code header here: https://github.com/apache/activemq-nms-openwire/pull/24


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq-nms-openwire] brudo commented on pull request #23: Updates to help move forward NMS openwire 2.0.0 release

Posted by GitBox <gi...@apache.org>.

brudo commented on PR #23:
URL: https://github.com/apache/activemq-nms-openwire/pull/23#issuecomment-1165878579

   Oh, shoot - I hasdn't seen that existing #21 where it was already done, to make it "None". But there are other fixes included in the current PR, to clear up some compile-time warnings.
   
   Should I remove the SslContext commits (and maybe also the SharpZip version increment, which the dependabot picked up in #22)? Or should I close this and submit a new PR with just the changes that were not in #21 or #22? Or do you want to merge this PR with all squashed together, with a commit message that conforms to https://cwiki.apache.org/confluence/display/GEODE/Commit+Message+Format?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org