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

SFI images IIS-docs sev1-2 updates only #1072

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

wadepickett
Copy link
Contributor

@wadepickett wadepickett commented Oct 28, 2024

Fixes dotnet/AspNetCore.Docs#33750

All SFI image updates to fix sev 1-2 issues per SFI issue instruction.

Using these guidelines:

  • Leave in the loopback address, typically 127.0.0.1.
  • Leave in "Administrator" when it is used as the user name even though flagged.
  • When details like a local path is left in and flagged, keep those anyway.
  • Leave in completely obscured passwords, (replaced with dots) even though they are called out.
  • Leave in the name of a local database used for the article, like "School" or "Blogging" or "AdventureWorks."
  • Leave in localdb or SQLExpress used as server name. Consider leaving in localdb connection strings.

@wadepickett wadepickett marked this pull request as ready for review October 28, 2024 15:02
@wadepickett wadepickett requested a review from a team as a code owner October 28, 2024 15:02
@wadepickett wadepickett requested a review from tdykstra October 28, 2024 15:02
Copy link
Collaborator

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

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

It seems extreme to blank out localhost IP addresses, but if that's what we're directed to do, this PR LGTM

@Rick-Anderson
Copy link
Contributor

It seems extreme to blank out localhost IP addresses, but if that's what we're directed to do, this PR LGTM

I'd like to pushback and get some reason why. There's many tools and operations that rely on the loopback IP

Copy link
Contributor

Choose a reason for hiding this comment

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

What does the doc look without these? Why can't you issue a warning although I don't know what warning that would be. This is a tool that requires PW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

provide the link to this doc

Copy link
Contributor Author

@wadepickett wadepickett Oct 28, 2024

Choose a reason for hiding this comment

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

@Rick-Anderson
Copy link
Contributor

@John-Hart (or his delegate) approval needed before merging this

@Rick-Anderson
Copy link
Contributor

@wadepickett take a look at how the Azure folks are handling similar tools. They give a tepid informational message at the top of the article.

@Rick-Anderson
Copy link
Contributor

@wadepickett can you cite a reference to removing loop back IP?

@wadepickett
Copy link
Contributor Author

wadepickett commented Oct 28, 2024

Thanks for the suggestions and info:

We were instructed to remove what was called out, as a top priority, and IP's were specifically called out in those cases, so that is what I was doing. To me, any example simply used as an fictional example, which would be 99% of all of the doc issues, would be an odd high bar to hit , but they were intentionally flagging fictional examples from what I could see. Almost all the flags are for fictional examples for all doc repos.

Rick, do you have a link to an example you like where the images are not addressed as requested, but instead they use an informational message at the top of the doc that you are referring to on Azure?

Who do we push back to? The SFI engineers?

The images were updated, not deleted and gone. For binaries, they are replaced when you update and there is no way around that. It is confusing when you refer to images as just "deleted" when they have been updated. It would be helpful if you communicate which you are referring to.

@Rick-Anderson
Copy link
Contributor

We were instructed to remove what was called out,

Where were we instructed to blindly remove everything flagged? AFAIK, we were instructed to review everything flagged. Unless you can prove the loop back IP is a risk, it should be reviewed and exempted.

Almost all the flags are for fictional examples for all doc repos.

Loop back is not fictional, it's an IP needed to for loop-back.

You can replace fictional IP's with an IP from the list of approved IP's, just as with GUIDs

@Rick-Anderson
Copy link
Contributor

Rick-Anderson commented Oct 28, 2024

@wadepickett

rick, do you have a link to an example

I don't think Rick Bradley cares
See https://github.com/MicrosoftDocs/reusable-content/pull/213/files
ROPC is significantly higher risk that IP's

they were intentionally flagging fictional examples from what I could see.

Loop-back is not fictional. IP's are different than GUIDs

@Rick-Anderson
Copy link
Contributor

The first one is fine but I question why you need to make any changes. The PW is obscured. Admin is used through out the docs.

You 've left an obscured PW here, which is fine because it's obscured.

image

@Rick-Anderson
Copy link
Contributor

You've left in the text, which can be copy/pasted. You can't do that easily with an image. It's inconsistent.

Specify FTP IP filtering options that allow access from

127.0.0.1

and deny access from the

169.254.0.0/255.255.0.0

range of IP addresses.

<location path="ftp.example.com">
  <system.ftpServer>
    <security>
      <authorization>
        <add accessType="Allow" roles="administrators" permissions="Read, Write" />
      </authorization>
      <requestFiltering>
        <fileExtensions allowUnlisted="true">
          <add fileExtension=".exe" allowed="false" />
          <add fileExtension=".bat" allowed="false" />
          <add fileExtension=".cmd" allowed="false" />
        </fileExtensions>
        <requestLimits maxAllowedContentLength="1000000" maxUrl="1024" />
        <hiddenSegments>
          <add segment="_vti_bin" />
        </hiddenSegments>
      </requestFiltering>
      <ipSecurity enableReverseDns="false" allowUnlisted="true">
        <add ipAddress="127.0.0.1" allowed="true" />
        <add ipAddress="169.254.0.0" subnetMask="255.255.0.0" allowed="false" />

@wadepickett
Copy link
Contributor Author

wadepickett commented Oct 28, 2024

The first one is fine but I question why you need to make any changes. The PW is obscured. Admin is used through out the docs.

I was trying to make the best of the whole awkward situation in each case. The way the dialog was introduced, it felt like the dialog fields could be in a state of not filled out yet. We were asked to address the first few entered fields, user name etc, and it seemed out of state to leave in the obsured passwords at the bottom.

image

I can redo to leave the dialog half filled, with obscured passwords in there. Is that what you want if the other fields entries do need to be removed.

Since the loopback IP needs to stay there in every case, for which I will revert. It might be faster to call out the ones you feel actually need udpates which I will follow for any such cases.

As far as some message at the top of the doc, as you say, what would that be? I can't think of one that is appropriate either. If these are flags that simply should not be flagged I can not that, adjust the PR and update the spreadsheet to note they were reviewed and will not be changed.

@Rick-Anderson
Copy link
Contributor

We were asked to address the first few entered fields, user name

Asked by whom?
the user name Administrator is used throughout the docs. The security is in the PW, not in security by obscurity on the user name.

@wadepickett
Copy link
Contributor Author

wadepickett commented Oct 28, 2024

Maybe we can break this down into some guidelines I can follow here. I am not that familier with the IIS repo.

  • Leave in the loopback address, typically 127.0.0.1.
  • Leave in "Administrator" when it is used as the user name even though flagged.
  • When details like a local path is left in and flagged, keep those anyway.
  • Leave in completely obscured passwords, (replaced with dots) even though they are called out.

any other guidelines you see here from the image list I could add before redoing?

@Rick-Anderson
Copy link
Contributor

I don't care that
image

was changed, I just question the need to change it.
The loop back shouldn't be changed

@Rick-Anderson
Copy link
Contributor

Rick-Anderson commented Oct 28, 2024

This walkthrough contains a series of steps in which you log in to your FTP site using an IIS Manager account. These steps should only be followed on the server itself using the loopback address or over SSL from a remote server.

IIS Manager is designed to manage websites and apps hosted on the local machine. By default, IIS creates bindings that listen on the loopback address, allowing access to websites on the local machine using http://localhost or http://127.0.0.1.

@rick rick mentioned this pull request Oct 28, 2024
@Rick-Anderson
Copy link
Contributor

rick/you-rang#1

Ha ha, be sure to read that @wade

@wadepickett
Copy link
Contributor Author

wadepickett commented Oct 28, 2024

@Rick-Anderson @tdykstra :

Maybe we can break this down into some guidelines I can follow here. I am not that familier with the IIS repo.

  • Leave in the loopback address, typically 127.0.0.1.
  • Leave in "Administrator" when it is used as the user name even though flagged.
  • When details like a local path is left in and flagged, keep those anyway.
  • Leave in completely obscured passwords, (replaced with dots) even though they are called out.

Any other guidelines you see here from the image list I could add before redoing?

@wadepickett
Copy link
Contributor Author

wadepickett commented Oct 28, 2024

they were intentionally flagging fictional examples from what I could see.

My point being that the bar for what they would allow seemed very high, not that the loopback address is fictional, nor that an IP is the same as a GUID.

@tdykstra
Copy link
Collaborator

Any other guidelines you see here from the image list I could add before redoing?

  • Leave in the name of a local database used for the article, like "School" or "Blogging" or "AdventureWorks."
  • Leave in localdb or SQLExpress used as server name. Consider leaving in localdb connection strings.

@wadepickett
Copy link
Contributor Author

Any other guidelines you see here from the image list I could add before redoing?

  • Leave in the name of a local database used for the article, like "School" or "Blogging" or "AdventureWorks."
  • Leave in localdb or SQLExpress used as server name. Consider leaving in localdb connection strings.

Thanks! That helps me get a better idea of where the line is compared to what is getting flagged. Much appreciated!

@Rick-Anderson
Copy link
Contributor

My point being that the bar for what they would allow seemed very high

Where, that's not what I read.

@wadepickett
Copy link
Contributor Author

My point being that the bar for what they would allow seemed very high

Where, that's not what I read.

the bar for what they would allow seemed very high given what they were flagging.

@MicrosoftDocs MicrosoftDocs deleted a comment from fatmasamuk Oct 28, 2024
@Rick-Anderson
Copy link
Contributor

the bar for what they would allow seemed very high given what they were flagging.

You don't understand flagging. Flagging means take a look, it's not a directive to delete content.

The bar is very low for flagging.

Copy link
Contributor Author

@wadepickett wadepickett Nov 3, 2024

Choose a reason for hiding this comment

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

Inline image comments: Preview in doc

Details flagged are not used specifically for the tutorial and the state of the image can represent a user not selected yet as opposed to one selected given the tutorial context where it is used.

Copy link
Contributor Author

@wadepickett wadepickett Nov 3, 2024

Choose a reason for hiding this comment

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

Inline image comments: Preview in doc

The Data Channel Port range is a fictitious range specific to the tutorial and needs to show it in a greyed-out state, so I left it in even though it was flagged. However, the External IP address does not need to be in the dialog yet at this point and this specific random IP has no significance in the tutorial. Since an example is already provided in the dialog below the field in case there is any confusion about what would go in there so removed that item that was flagged.

Copy link
Contributor Author

@wadepickett wadepickett Nov 3, 2024

Choose a reason for hiding this comment

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

Inline image comments: Preview in doc

I used grey out instead of matching black to make it clear there is information that would be there but has been blanked out. The Hostname:port is essential to the tutorial and called out, plus is fictional, so I left that in but removed the Cert hash, app id and cert store name since those were not referenced in the tutorial.

Copy link
Contributor Author

@wadepickett wadepickett Nov 4, 2024

Choose a reason for hiding this comment

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

Inline image comments: Preview in doc

Removed cert name that does not need to be present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inline image comments: Preview in doc

I used grey out instead of matching black to make it clear there is information that would be there but has been blanked out. The Hostname:port is essential to the tutorial and called out, plus is fictional, so I left that in but removed the Cert hash, app id and cert store name since those were not referenced in the tutorial.

@wadepickett wadepickett merged commit 59e4aed into main Nov 5, 2024
2 checks passed
@wadepickett wadepickett deleted the wadepickett/2ndTry33750SFIimageFixOnly branch November 5, 2024 00:00
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.

SFI - Images - iis-docs repo - Fix all sev 1 - 2
3 participants