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.

Sign up for GitHub

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

Describe limitations of auth-user-pass more clearly #172

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Describe limitations of auth-user-pass more clearly #172

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion doc/man-sections/client-options.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,13 @@ configuration.
auth-user-pass up

If ``up`` is present, it must be a file containing username/password on 2
lines. If the password line is missing, OpenVPN will prompt for one.
lines. If the password line is missing, OpenVPN will prompt for one on
stdin.

*NOTE*: If you use a graphical OpenVPN client such as OpenVPN GUI on Windows,
you should either define both the username and password in the file, or rely
Copy link
Member

Choose a reason for hiding this comment

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

The NOTE: should be on a single line, and the text should be indented one more level to be correctly rendered.

The current patch gives:

              If up is present, it must be a file containing username/password
              on 2 lines. If the password line is missing, OpenVPN will prompt
              for one on stdin.

              NOTE: If you use a graphical OpenVPN client such as OpenVPN  GUI
              on Windows, you should either define both the username and pass‐
              word in the file, or rely on the GUI  caching  the  credentials;
              defining just the username in the file will not work.

              If  up  is  omitted, username/password will be prompted from the
              console.

With suggested changes:

              If up is present, it must be a file containing username/password
              on 2 lines. If the password line is missing, OpenVPN will prompt
              for one on stdin.

              NOTE:  If you use a graphical OpenVPN client such as OpenVPN GUI
                     on  Windows,  you  should either define both the username
                     and password in the file, or rely on the GUI caching  the
                     credentials; defining just the username in the file

              If  up  is  omitted, username/password will be prompted from the
              console.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you look at server-options.rst you see the same style I've used with NOTE sections:

  *NOTE*: to *change* an option, ``--push-remove`` can be used to first
  remove the old value, and then add a new ``--push`` option with the new
  value.

  *NOTE 2*: due to implementation details, 'ifconfig' and 'ifconfig-ipv6'
  can only be removed with an exact match on the option (
  :code:`push-remove ifconfig`), no substring matching and no matching on
  the IPv4/IPv6 address argument is possible.

They also render the same way as my version. Do we want to do a major surgery in addition to an implant? 😂

Copy link
Member

Choose a reason for hiding this comment

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

Right, there is some inconsistency there. I think we should move all of these NOTE/WARN/INFO (whatever tags we use) to an indented approach, to hightlight it better.

This also gives us better flexibility when we start adding some CSS styling to the HTML rendering, where the indented variant rendering gives much better abilities the formatting and styling. Essentially it changes from a more plain <p>...</p> for "everything" to a <dl><dt>$HEADER</dt><dd>$CONTENT</dd></dl> style.

Copy link
Member

@ordex ordex Sep 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@dsommers should we keep this patch as is and then reformat all the NOTEs with a follow up?

Copy link
Member

Choose a reason for hiding this comment

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

@mattock would you mind rebasing this patch on master and sending it to the mailing list? Once that's done, we can close this PR. Thanks!

on the GUI caching the credentials; defining just the username in the file
will not work.

If ``up`` is omitted, username/password will be prompted from the
console.
Expand Down