remove Application Data from path to I2P service config on Windows

requested review from @zzz

assigned to @idk

changed the description

added 1 commit

ecd58a83 - If default computed path contains %programdata% or system32 we're running as a...

Compare with previous version

added 1 commit

16eb1577 - remove extraneous logging

Compare with previous version

added 1 commit

2fd01c83 - add correct programdata path to default eepsite

Compare with previous version

%ProgramData%/Application Data/ isn't a "real" thing anymore, in modern Windows it's supposed to be a link back to %ProgramData%. Most of the time this works fine, but when we expressly set resourceBase in base-context.xml, that path gets used by Jetty which does not resolve %ProgramData%/Application Data/ back to %ProgramData%, resulting in it looking for files in a directory which does not exist, resulting in the 404. If we just remove references to Application Data(Which became a link to %ProgramData% in Windows Vista) all the paths agree and the 404 is fixed.

jetty doesn't follow it because it is 1. A symlink 2. A hidden file 3. a system file which Windows makes invisible to applications including explorer(all three)

Tested to fix the issue.

reviewing now. Gitlab email notifications appear to be broken again.

tried a symlink for the base-context.xml resourceBase on linux and it worked fine, so I don't think it's 1). May not be 2) or 3) either, but 4) from your link: "The security permissions on the junction point explicitly prohibit listing files, which is why you can't get a listing of its contents"

please explain what combinations of OS version/installer 4 or 5/service or not/signed installer or not/ that:

you reproduced on
the problem exists on
you tested the fix on

Also, any symptoms of the problem other than 404? log messages on install, first run, or otherwise?

I noticed, should be coming back up with the next server update(When it picks up the config file again), gandi makes me talk to them about it it every six months so I can keep sending bulk email. Part of the reason I'm purging spam accounts more aggressively now.

The log messages in the wrapper on first install show files being installed to both %ProgramData%/i2p and %ProgramData%/Application Data/i2p.

I tested a fresh install on a Windows 11 virtual machine with the released install and Java 8. I installed it and with "Windows Service" checked, in order to reproduce the issue.

I built fresh installers using several ant targets and tried them as well:

Installers built with ant distclean installer5 works for user-installs but not for service installs.
Installers built with ant distclean installer works for user-installs but not for service installs.
Installers built with ant distclean installer-windows work for service installs and user installs. The bug happens here too.

I tested the fix using a exe built using ant distclean installer5-windows, also using a fresh install of Windows 11 on a VM with Java 8.

I also tested it on Windows 10 hardware, where I uninstalled an existing router(user-install) and installed the ant distclean installer5-windows .exe.

any symptoms of the error other than 404? logs?

Not quite sure what the intent of the WorkingDir changes is.

Current code sets to LOCALAPPDATA first, then switches to APPDATA if it meets a bunch of sanity checks.

What's the intended logic of the changes?

The logs show files being installed to both locations even though only one of them technically exists(before my changes) and also indicate that the file was not found when the 404 occurs. I didn't see any other indications.

Re: WorkingDir some of the changes there are actually several years old. And there's backstory:

So, many years ago, we installed all our working config files into %APPDATA% because that was what was normal at the time and there was no reason not to. For the vast majority of home PC users running Windows, there was no functional difference between %APPDATA% and %LOCALAPPDATA% because home user's didn't have anything to make their "Roaming" application data profile actually "roam" to the other computers where they logged in. This feature was almost exclusively used by groups where somebody's full-time job included making sure that the right files were always available to the corporate accounts. So lots of applications, not just us, saw %APPDATA% and assumed that was the directory where they were supposed to place their config files.

XP came and went. Longhorn became Vista, came and went. Windows 7 came and went and a few people tried to hang on to 7 just a little longer partly because of one big change that MS started pushing aggressively with Windows 8.1... Microsoft Account Services and more specifically, OneDrive. After 8.1, Windows started "encouraging" and at times forcing users outright to sign up for a MS account with OneDrive when they set up the user account on their own PC, and one of the things OneDrive does is sync settings across PC's where you log into your account by actually making your roaming appdata directory roam. This is kind of cool and TBH could improve quality-of-life with many applications, but the sync is only end-to-middle encrypted, data at rest on MS servers is readable to MS. So if we install to roaming appdata by default, then Microsoft starts to get un-encrypted copies of people's I2P configs including the router keys, the tunnel keys, etc. It's basically giving MS the power to impersonate those routers.

So, I changed it to use %LocalAppData% on new installs instead, to prevent that syncing by default. But I didn't want to break existing installs that use %AppData%, and I didn't want to prevent people from using %AppData% on purpose if they wanted to. In order to do that I made the new default %LocalAppData% but check for a config that has clients and has actually been used in %AppData%, by checking for a clients.config and a clients.config.d directory. If the migration has happened, then the %AppData% config has been used before, and we keep using it. If not, we use %LocalAppData% instead because it is a saner default.

The only new change to WorkingDir is that if we detect that we're installing files to %ProgramData% i.e. a service install, home is always just %ProgramData% now, and never %ProgramData%/Application Data.

reproducing

From above:

Installers built with ant distclean installer5 works for user-installs but not for service installs.
Installers built with ant distclean installer works for user-installs but not for service installs.
Installers built with ant distclean installer-windows work for service installs and user installs. The bug happens here too.

the last one confuses me - you say "work" but also "bug". Don't understand. Why?

OP says you couldn't reproduce with the release installer (5). Why?

root cause

Only service installs use Application Data for the config dir.

Claim above and in SO is that Application Data is a symlink or "junction" to Program Data for "modern windows". What version is "modern"? Which versions have the symlink and which don't? Does are code need to handle both cases or only "modern"? On what systems are they the same and on what are they different? which will inform the following:

WorkingDir change comments

There's a lot of strangeness in these few lines.

No check for programData == null, it will NPE
Why the conversion to lower case before comparison?
Why use contains() and not startsWith() for the comparison?
"system32" is already lower case!
This is after all the sanity checks for appData existing. Do we need similar checks here, or should all this be before the appData checks? Or:
should we just check if getCanonicalPath() is the same for both?

Workaround

We'll need to post a workaround for existing users, which is what? Edit base-context.xml? Is that it? Need to test

Re: Reproducing:

When I tested installers built with ant installer and ant installer5 I ended up with an .exe which was roughly 8MB larger than the one for ant installer-windows and ant installer5-windows. The installers built with ant installer and ant installer5 were not capable of successfully completing a service install at all.

ant installer-windows and ant installer5-windows successfully complete the service install, but also both exhibit the missing eepSite files bug.

Re: Modern Windows

The change from %ProgramData%/Application Data/ to %ProgramData% appears to have been introduced in the Vista era. I can find people were asking for help on Stack Overflow resolving issues related to this specific junction point who state that they are using Windows 7 as far back as 12 years ago. I know for sure it is not present on Windows 8.1, 10, or 11. If we need compatibility code for this, it's not for a version of Windows which is getting updates, even in paid support by Microsoft.

Re: Workaround

The workaround I tested was to change the path in base-context.xml to remove the /Application Data/ element in the path. Removing this path element and restarting the router fixes the issue.

Re: Strangeness

No check for programData == null, it will NPE - I'll add the check
Why the conversion to lower case before comparison? - Windows filesystems are case-insensitive, so I made the comparison case-insensitive to match
Why use contains() and not startsWith() for the comparison? - Because "system32" is not the whole prefix in this case because there's no environment variable for it. To get that it's actually %WINDIR%/system32 which is what I made it do instead in the last checkin.
"system32" is already lower case! - Since we're now checking startsWith %WINDIR%/system32 instead of contains system32 I kept the lowerCase
This is after all the sanity checks for appData existing. Do we need similar checks here, or should all this be before the appData checks? - No, because in the case of AppData/Roaming/i2p we're validating and using the directory only if we detect a config which has been run at least once before, whereas in %ProgramData%/i2p it's OK to copy over and create a completely new configuration.
should we just check if getCanonicalPath() is the same for both? - I don't know, I'll try it and see if it works.

added 1 commit

0ce816ac - change path validation for service installs when install location contains system32

Compare with previous version

installer and installer5 are 8MB bigger because they contain the linux and mac jbigi's. My release script uses installer5-windows. May not be worth investigating right now, but why are installer and installer5 "not capable" of service install??? What's the symptom? Strange...

re: workaround, great, we have consensus on that, we'll need to post that somewhere, probably zzz.i2p

re: modern, if it's been a junction since Vista, then can we make the fix simpler? would that be safer?

The key symptom is that the service does not start due to it looking in the wrong directory, of all places, it looks in the user's %localappdata% and is denied access, and the service fails to start. This applies to starting the service from the installer and from services.msc

Re: a simpler fix, I think it should be fine if we just make sure that we don't configure it to use %programdata%/Application Data anywhere. If it isn't told to explicitly, it doesn't try to use it so this whole rigamarole is just that unnecessary.

It doesn't look like we actually configure it to use /Application Data/ outside of those specific config files where we expressly set it from my first checkin above. Everywhere else, we either look at %programdata% or %allusersprofile% which both point to the same place which is C:/ProgramData/. If we go the simpler route without the sanity check, I think the references to %allusersprofile% should all be changed to %programdata%for the sake of consistency.

re: workaround: Additional files to fix: cgi-context.xml, jetty.xml, jetty-ssl.xml

re: "works" vs. "bug", still not clear, when you say an installer "works for user-installs but not for service installs" do you mean some problem UNRELATED to the bug at hand? I'm still struggling here.

re: root cause / reproducing: Do you understand why the signed 1.9.0 release installer works?

re: WorkingDir changes: Still struggling to understand the intent. Because:

If i2p.dir.config is set, the entire code block starting at line 80 is skipped
i2p.dir.config is always set for service install, thanks to set_config_dir_for_nt_service.bat and that a service install is always started via the wrapper (never runplain)
Even if the above weren't true, for an existing install (service or not), changing i2p.dir.config value doesn't help because the paths in all the jetty config files is still wrong
Even if the above weren't true, for a new install (service), the set_config_dir_for_nt_service.bat change is sufficient to fix it
Even if the above weren't true, for a new install (non-service) it's going to go to LOCALAPPDATA, because all the checks to see if there's an existing install in APPDATA will fail.

what am I missing? prove me wrong...

works vs bug: as far as I can tell it is unrelated to the bug at hand. I only found it because I was building with ant installer instead of ant installer5-windows. I don't yet know why the latter would work and not the former. I do know that changing set_config_dir_for_nt_service.bat does not fix it.

I don't think I can prove you wrong, I think we agree. The only changes that are actually necessary to fix this specific issue for new service installs built with ant installer5-windows is set_config_dir_for_nt_service.bat. That's all I did when I opened the MR at first: https://i2pgit.org/i2p-hackers/i2p.i2p/-/merge_requests/65/diffs?commit_id=b533ccaabd3364cbd8bfe303e1b80cf54145ab89

I don't think the changes I made in WorkingDir do anything in any scenario we configure with the installer. They would only apply if a service install were in use and the i2p.dir.config is unset, a situation which does not exist by default and is unlikely to exist.

The only point I would like to redirect you on is the last one. The switching logic between %LOCALAPPDATA and %APPDATA% is not for new installs, it's specifically there to handle upgrades, wherein the old default in %APPDATA% had previously been in use, but i2p.dir.config is also unset. For new installs it will always go to %LOCALAPPDATA, the only time it won't use it is if those checks pass because an already-existing %APPDATA% config.

verified on my 8.1 laptop and win 10 box that %ALLUSERSPROFILE% is \ProgramData, containing Application Data as a JUNCTION, only visible with DIR \A. That's all consistent with the above.

The shortcutSpec.xml change is also required.

Here's the previous change to the set_config_dir... script:

commit 9e6f993a Author: kytv kytv@mail.i2p Date: Wed Aug 31 13:11:23 2011

De-fuglify the service path in Windows

The default service path in Windows is fugly and not very convenient. I2P uses
the correct path, but if you want to access snark or eepsite data, one must go to
%SYSTEMROOT%\config\systemprofile\AppData\Roaming\I2P\ (Vista/7) or
%SYSTEMROOT%\system32\config\systemprofile\Application Data\I2P (XP/2003). If
this wasn't bad enough, in some cases one must take ownership of this path and
grant permission to him- or herself to access the folder.

With this changeset, I'm setting the path to %ALLUSERSPROFILE%\Application
Data\I2P as well as adding a shortcut to the I2P folder in the Start menu.

yeah you're right, i2p.dir.config is not set for non-service install, so the code starting at line 80 will run for that, whether new or existing install.

added 1 commit

1c67f06f - Since service installs always have an i2p.dir.config set, they don't need to...

Compare with previous version

I removed the WorkingDir changes that were specific to service installs and changed %allusersprofile% to %programdata in the batch script and the shortcutSpec.xml, tested on my Windows 11 VM's and Windows 10 desktop.

It occurred to me that the description of the issue I had generating a successful installer to test with in the OP is the same issue as the one I am describing with ant installer and ant installer5. It seems that when you use an installer built with the ant installer5 target, even when you tell it to use a Windows service, it will look in the directory used for user installs. The most likely reason for that is because the i2p.dir.config is set incorrectly or unset. I don't know why yet, though.

what remains in WorkingDir appears to be debug logging changes and renaming a variable for no reason. Why not revert the whole file?

The logging changes were useful to observe where errors occurred when copying the files. Since there is still the issue with the installer5 and installer targets I left those changes in so that I could use the changed debugging messages when I'm trying to figure that out. I'd like to leave those in. I left the change which uses different variables for appdata and localappdata during the phase where it checks for a config in the default location only because I think it looks slightly clearer to me.

ok

do you have any feedback from the reporter (RN) either with the workaround (changing path in 4 jetty files) or a test installer (would have to remove the old install first, may not be willing) ?

p.s. sorry for delay, I saw you said on IRC that email was fixed, but still not getting any

added installer label

Yeah it worked for a little while then it stopped again, something different is happening this time though. Working on figuring out what part is wrong. I usually try to keep gitlab changes to the evening in case I need to reboot, I just tried a few that I think will work, I guess this post will tell.

I'll get with RN as soon to figure out whether the workarounds work for him. If I remember correctly, the Windows service he was trying to set up was intended as a backup, while he upgraded another system, so hopefully he can use the backup system to test.

OK looks like I've got them working again for now. I'll keep on keeping an eye on them.

changed milestone to %2.0.0

bump, let's not forget to work with RN to get this resolved in time for the release

Feedback from RN on the bug, I think we will need to add the i2ptunnel.config.d file to the workaround instructions, going to test and make sure that it doesn't occur on new installs but I don't think it should.

(01:35:52 PM) ReturningNovice: let me take a look
(01:39:44 PM) ReturningNovice: ok, in jetty.xml it is only in a comment line describing where stuff goes
(01:40:28 PM) ReturningNovice: this is in /program files/
(01:40:56 PM) ReturningNovice: this is in /program files/i2p/eepsite
(01:41:53 PM) ReturningNovice: path not in base-context.xml
(01:42:01 PM) ReturningNovice: I get the feeling I am looking in the wrong place
(01:42:52 PM) ReturningNovice: not in cgi-context
(01:44:28 PM) ReturningNovice: I searched all those files for "program" and it is only mentioned in comments.
(01:49:40 PM) ReturningNovice: searched instead for C: and that is not in any of those files
(01:51:30 PM) ReturningNovice: looking in %program data% now
(01:56:53 PM) ReturningNovice: OK, replaced all occurences in those files in %program data%
(02:01:21 PM) ReturningNovice: Is checking 127.0.0.1:7658 sufficient? it seems to be working.
(02:20:46 PM) ReturningNovice: ProgramData\Application Data also found in i2ptunnelconfig.d
(02:29:02 PM) ReturningNovice: Can't get it to use the privkey properly.
(02:29:06 PM) eyedeekay: Sorry had to handle something, it's the one in %programdata%
(02:29:23 PM) eyedeekay: 7658 should be sufficient
(02:29:31 PM) ReturningNovice: did a bit of side by side comparison and changed a few things to match, but no luck with the dest
(02:30:01 PM) ReturningNovice: I have an old private key, and an alt eddsa
(02:30:11 PM) eyedeekay: Shoot, then there's still something pointing to the wrong place I think?
(02:30:19 PM) ReturningNovice: no
(02:30:31 PM) ReturningNovice: it is because of ancient signing algo
(02:30:39 PM) ReturningNovice: 127.0.0.1:7658 is working
(02:30:57 PM) ReturningNovice: but the newer router won't let me chage from eddsa
(02:31:03 PM) ReturningNovice: maybe I need to restart the router
(02:31:22 PM) eyedeekay: Might be, I don't think the xml file changes will apply until you do
(02:33:52 PM) ReturningNovice: well, still unable to get the sigtype to dsa
(02:34:43 PM) ReturningNovice: I could use the eddsa key instead... let me try that
(02:35:39 PM) ReturningNovice: got it!
(02:35:55 PM) ReturningNovice: it was missing eepsite/  in front of the key file in the tunnel manager
(02:37:13 PM) ReturningNovice: you did see I also found that path in annother file?
(02:47:27 PM) ReturningNovice: yeah, I had restarted for the xml changes... it was my old dsa key that was giving problems because of user typo

ACK

As far as workaround instructions, a post on zzz.i2p should be sufficient. If lots of people were hitting it we would have heard about it long ago.

Please rewrite the checkin comments for the merge to describe the actual problem and changes; OP above is pretty speculative

approved this merge request

changed the description

merged

mentioned in commit 144271e4

remove Application Data from path to I2P service config on Windows

Merge request reports

Activity

reproducing

root cause

WorkingDir change comments

Workaround

Re: Reproducing:

Re: Modern Windows

Re: Workaround

Re: Strangeness