Closed moraja closed 6 months ago
PiJoCoder
commented
7 months ago This is awesome work, @moraja! I have started review and will be testing today and this week also. Could you upgrade to using the latest version of Azure.Identity in this PR to avoid vulnerabilities? See smiliar request in https://github.com/microsoft/SqlNexus/pull/269
PiJoCoder
commented
7 months ago @moraja I have several admin things that I'd like to ask you about.
moraja_reportviewer_Microsoft_data that you probably intended to use for this; could you switch to using that new branch and transfer all the changes to that branch? Thank you
PiJoCoder
commented
7 months ago I like that you have added the encryption logic on the Connect screen - great improvement! Could we change the default checkboxes so the connection works by default? Right now with Encrypt checked and Trust Server Certificate unchecked, it would fail on all servers that don't have SSL enabled.
Could we either have both unchecked or both checked (preferrable) in which case connecting will work off the bat.
PiJoCoder
commented
7 months ago Tested functionality and examined DLLs loaded in the process - things work well. The only downside is that we now have a much larger SQLNexus folder full of binaries, but we knew that we will get this as a result of this work. The up-side is that users won't have to install 2 of the prerequisites any longer - ReportViewer control and the SQLClrTypes. Great work.
moraja
commented
7 months ago I like that you have added the encryption logic on the Connect screen - great improvement! Could we change the default checkboxes so the connection works by default? Right now with Encrypt checked and Trust Server Certificate unchecked, it would fail on all servers that don't have SSL enabled.
Could we either have both unchecked or both checked (preferrable) in which case connecting will work off the bat.
The default behavior I used was to enable Trust Certificate by default, but then I remembered that SqlNexus is a public tool and trusting the certificate is a security risk that only the end user should be responsible for.
if a corporate certificate was truly compromised, we don't want to automatically accept that. that is my logic for making it non-default
moraja
commented
7 months ago Tested functionality and examined DLLs loaded in the process - things work well. The only downside is that we now have a much larger SQLNexus folder full of binaries, but we knew that we will get this as a result of this work. The up-side is that users won't have to install 2 of the prerequisites any longer - ReportViewer control and the SQLClrTypes. Great work.
I'm hoping that one day we will upgrade RML utilities to an easy to compile environment and directly ship needed modules with Nexus without having to install anything separately... but one can only live on hope :)
Thanks for the great feedback :)
PiJoCoder
commented
7 months ago I like that you have added the encryption logic on the Connect screen - great improvement! Could we change the default checkboxes so the connection works by default? Right now with Encrypt checked and Trust Server Certificate unchecked, it would fail on all servers that don't have SSL enabled. ![image] Could we either have both unchecked or both checked (preferrable) in which case connecting will work off the bat.
The default behavior I used was to enable Trust Certificate by default, but then I remembered that SqlNexus is a public tool and trusting the certificate is a security risk that only the end user should be responsible for.
if a corporate certificate was truly compromised, we don't want to automatically accept that. that is my logic for making it non-default
Yes, I think it's up to the user's admins of environment to ensure proper back-end security for SQL Server - thus trusting a certificate is up to the end user. Howver, having Encrypt Connection by default without trust server certificate will fail in most cases by default if no SSL. To have to uncheck the box every time would be annoying for me as a user and with these current defaults (and largest user base is SQL engineers), I'll have to do this every time I use SQL Nexus. Possibly a good solution is to give users choice: for example if you would like to keep that configuration as default with encryption always enabled, we could consider saving these settings and preserve them for the user. SQL Nexus doing that for multiple things including Import settings and Login settings and saving them in a user config file. See
https://github.com/microsoft/SqlNexus/blob/a9b8fb699f5de9156776b100d00355ff3a19629f/sqlnexus/fmConnect.cs#L120 https://github.com/microsoft/SqlNexus/blob/a9b8fb699f5de9156776b100d00355ff3a19629f/sqlnexus/fmLoginEx.cs#L56 https://github.com/microsoft/SqlNexus/blob/a9b8fb699f5de9156776b100d00355ff3a19629f/sqlnexus/Utilities.cs#L32
PiJoCoder
commented
7 months ago Tested functionality and examined DLLs loaded in the process - things work well. The only downside is that we now have a much larger SQLNexus folder full of binaries, but we knew that we will get this as a result of this work. The up-side is that users won't have to install 2 of the prerequisites any longer - ReportViewer control and the SQLClrTypes. Great work.
I'm hoping that one day we will upgrade RML utilities to an easy to compile environment and directly ship needed modules with Nexus without having to install anything separately... but one can only live on hope :)
Thanks for the great feedback :)
We can get it done sooner rather than later, but a lot of things before that need to take place
moraja
commented
7 months ago @moraja I have several admin things that I'd like to ask you about.
- This seems to be an old branch from your previous PR that contains commits that we have already included in the previous PR, also the branch name doesn't match the work. I see you have created a new branch
moraja_reportviewer_Microsoft_datathat you probably intended to use for this; could you switch to using that new branch and transfer all the changes to that branch?- Also, could you please associate the each commit message with a Github issue by using the #IssueID syntax. That way we can track which commit resolved what issue and it's easier to see code changes in the future. You will find it very useful yourself in the future as well as it will help others.
Could you provide in the PR description section instructions on
- how to test the PR
- what to install/configure for the PR (in this case, I assume we need instructions on which version of Nuget ReportViewer Control and Microsoft.Data.SQLCLient to install. You can provide the command or screenshots. These will also come in handy in the future when other developers need to know how to upgrade their packages. Please note that there are 94 forks of this repo and it would behoove us to assist the developers in those forks.
Thank you
I did work on the wrong branch by mistake, Now I cherry picked everything into the right branch if this makes a difference... I can cancel this PR and create a new one.
Nuget packages should not need anything extra, they download with the project, so no special instructions there.
for testing the only area that may have needed special instructions is the login screen, otherwise the changes do not introduce anything, testing should be simply running full load and everything succeeds without hiccups.
I will try to add the commits to the tasks now
moraja
commented
7 months ago I like that you have added the encryption logic on the Connect screen - great improvement! Could we change the default checkboxes so the connection works by default? Right now with Encrypt checked and Trust Server Certificate unchecked, it would fail on all servers that don't have SSL enabled. ![image] Could we either have both unchecked or both checked (preferrable) in which case connecting will work off the bat.
The default behavior I used was to enable Trust Certificate by default, but then I remembered that SqlNexus is a public tool and trusting the certificate is a security risk that only the end user should be responsible for. if a corporate certificate was truly compromised, we don't want to automatically accept that. that is my logic for making it non-default
Yes, I think it's up to the user's admins of environment to ensure proper back-end security for SQL Server - thus trusting a certificate is up to the end user. Howver, having Encrypt Connection by default without trust server certificate will fail in most cases by default if no SSL. To have to uncheck the box every time would be annoying for me as a user and with these current defaults (and largest user base is SQL engineers), I'll have to do this every time I use SQL Nexus. Possibly a good solution is to give users choice: for example if you would like to keep that configuration as default with encryption always enabled, we could consider saving these settings and preserve them for the user. SQL Nexus doing that for multiple things including Import settings and Login settings and saving them in a user config file. See
done :)
PiJoCoder
commented
7 months ago @moraja I have several admin things that I'd like to ask you about.
- This seems to be an old branch from your previous PR that contains commits that we have already included in the previous PR, also the branch name doesn't match the work. I see you have created a new branch
moraja_reportviewer_Microsoft_datathat you probably intended to use for this; could you switch to using that new branch and transfer all the changes to that branch?- Also, could you please associate the each commit message with a Github issue by using the #IssueID syntax. That way we can track which commit resolved what issue and it's easier to see code changes in the future. You will find it very useful yourself in the future as well as it will help others.
Could you provide in the PR description section instructions on
- how to test the PR
- what to install/configure for the PR (in this case, I assume we need instructions on which version of Nuget ReportViewer Control and Microsoft.Data.SQLCLient to install. You can provide the command or screenshots. These will also come in handy in the future when other developers need to know how to upgrade their packages. Please note that there are 94 forks of this repo and it would behoove us to assist the developers in those forks.
Thank you
I did work on the wrong branch by mistake, Now I cherry picked everything into the right branch if this makes a difference... I can cancel this PR and create a new one.
Nuget packages should not need anything extra, they download with the project, so no special instructions there.
for testing the only area that may have needed special instructions is the login screen, otherwise the changes do not introduce anything, testing should be simply running full load and everything succeeds without hiccups.
I will try to add the commits to the tasks now
Thanks. Let's keep it in this PR for now given the work you've done already. The Nuget packages don't install for me automatically. Perhaps I don't know something. But for example, a build fails for me currently because it appears I don't have the x64 version of SQL spatial DLLs. I guess I have to track that down and install with Nuget?
Perhaps we can do a screen share and debug
This is an example of the commands I used for RVC Nuget package
NuGet\Install-Package Microsoft.ReportingServices.ReportViewerControl.Winforms -Version 150.1586.0@moraja one more question - do all projects need the Azure.Identity and Azure.Core Nuget packages installed? I think perhaps the Rowset Editor and SQL Nexus only? Or are these getting installed by Microsoft.Data.SqlClient?
PiJoCoder
commented
7 months ago I like that you have added the encryption logic on the Connect screen - great improvement! Could we change the default checkboxes so the connection works by default? Right now with Encrypt checked and Trust Server Certificate unchecked, it would fail on all servers that don't have SSL enabled. ![image] Could we either have both unchecked or both checked (preferrable) in which case connecting will work off the bat.
The default behavior I used was to enable Trust Certificate by default, but then I remembered that SqlNexus is a public tool and trusting the certificate is a security risk that only the end user should be responsible for. if a corporate certificate was truly compromised, we don't want to automatically accept that. that is my logic for making it non-default
Yes, I think it's up to the user's admins of environment to ensure proper back-end security for SQL Server - thus trusting a certificate is up to the end user. Howver, having Encrypt Connection by default without trust server certificate will fail in most cases by default if no SSL. To have to uncheck the box every time would be annoying for me as a user and with these current defaults (and largest user base is SQL engineers), I'll have to do this every time I use SQL Nexus. Possibly a good solution is to give users choice: for example if you would like to keep that configuration as default with encryption always enabled, we could consider saving these settings and preserve them for the user. SQL Nexus doing that for multiple things including Import settings and Login settings and saving them in a user config file. See https://github.com/microsoft/SqlNexus/blob/a9b8fb699f5de9156776b100d00355ff3a19629f/sqlnexus/fmConnect.cs#L120
done :)
You are a machine! :) Thank you
moraja
commented
7 months ago @moraja one more question - do all projects need the Azure.Identity and Azure.Core Nuget packages installed? I think perhaps the Rowset Editor and SQL Nexus only? Or are these getting installed by Microsoft.Data.SqlClient?
I tried to remove core-identity references, while I can do it form the project references and it won't impact anything, I still couldn't uninstall the nuget package because SqlClient still depends on it. I think we should keep it along with the references and add a task to include AAD login and AzureDB connectivity sometime in the future to SqlNexus as we modernize the product.
PiJoCoder
commented
7 months ago @hacitandogan, @asavioliMSFT do you guys want to test this before we merge?
Fixes #140 and #273 Moved sql calls to user Microsoft.Data.SqlClient Added Encryption and Trust Certificate options to login screen Removed deprecated calls Added ReportViewer support from Nuget packages.