-
Notifications
You must be signed in to change notification settings - Fork 319
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
Conversation
There was a problem hiding this 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
I'd like to pushback and get some reason why. There's many tools and operations that rely on the loopback IP |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to the staged doc containing this image:
https://review.learn.microsoft.com/en-us/IIS/configuration/configurationredirection?branch=pr-en-us-1072
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...ost/sites/site/ftpServer/security/authentication/anonymousAuthentication/_static/image13.png
Outdated
Show resolved
Hide resolved
@John-Hart (or his delegate) approval needed before merging this |
@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. |
@wadepickett can you cite a reference to removing loop back IP? |
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. |
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.
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 |
I don't think
Loop-back is not fictional. IP's are different than GUIDs |
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.1and deny access from the 169.254.0.0/255.255.0.0range of IP addresses.
|
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. 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. |
Asked by whom? |
Maybe we can break this down into some guidelines I can follow here. I am not that familier with the IIS repo.
any other guidelines you see here from the image list I could add before redoing? |
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. |
Ha ha, be sure to read that @wade |
Maybe we can break this down into some guidelines I can follow here. I am not that familier with the IIS repo.
Any other guidelines you see here from the image list I could add before redoing? |
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. |
|
Thanks! That helps me get a better idea of where the line is compared to what is getting flagged. Much appreciated! |
Where, that's not what I read. |
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. |
…ettings required for topics
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…ial as example and fictional
There was a problem hiding this comment.
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.
Fixes dotnet/AspNetCore.Docs#33750
All SFI image updates to fix sev 1-2 issues per SFI issue instruction.
Using these guidelines: