allow 127.0.0.1 in bindTo #851

ArneBab · 2023-08-29T20:27:51Z

For a Tor bridge that excludes all other IPs from the noderef to avoid spilling the real IP

src/freenet/node/NodeIPPortDetector.java

ArneBab · 2024-02-18T09:08:55Z

I now added a config option for this.

Bombe · 2024-02-20T11:05:30Z

src/freenet/node/NodeIPDetector.java

+
+			@Override
+			public void set(Boolean val) throws NodeNeedRestartException {
+				if (allowBindToLocalhost != val) {


This will throw a NullPointerException if val is null.

Bombe · 2024-02-20T11:06:28Z

src/freenet/node/NodeIPDetector.java

+			@Override
+			public void set(Boolean val) throws NodeNeedRestartException {
+				if (allowBindToLocalhost != val) {
+					synchronized(this) {


This synchronization is broken, as long as there are places where allowBindToLocalhost is accessed without synchronization, such as in the get() method.

Bombe · 2024-02-20T11:08:16Z

src/freenet/support/SortedIntSet.java

This change belongs to a different pull request, doesn’t it?

gits7r · 2024-02-21T19:50:44Z

Binding to localhost already works for darknet node, at least I can decode 127.0.0.1:port from my base64 encoded physical.udp line, so this feature is only related to opennet mode. In this case, shouldn't we register config as node.opennet.allowBindToLocalhost instead of Node.allowBindToLocalhost ?

We could add a comment that it should probably (not mandatory) be used along with node.opennet.includeLocalAddressesInNoderefs and node.opennet.alwaysAllowLocalAddresses, but those latter two config parameters will also work for other Local Addresses even if Bind to Localhost is set to false.

Also, would be good to add 0:0:0:0:0:0:0:1 and/or ::1 in addition to 127.0.0.1 everywhere in order to eliminate any confusion.

A sane description would be for this config param (of please someone make it shorter if possible, I'm not too good at shorting phrases):

"In Opennet mode it would be impossible for someone else to connect to your node using the localhost loopback address. This feature is needed if you use other applications that provides a reachable address of any sort and proxies that to localhost. Currently this node has no way to guess those kind of addresses and add them to your physical.udp line, and there might be no other peers that can reach certain type of addresses. Set this to "true" only if you know what you are doing".

It would be really neat if this would make it into the next. Covers at least few use cases reported on FMS.

Bombe · 2024-03-12T20:27:55Z

src/freenet/config/ConfigCallback.java

+	 * @param set accepts the new value and return an Exception or null (if no exception)
+	 */
+	public static <T> ConfigCallback<T> from(Supplier<T> get, Function<T, Exception> set) {
+		return new ConfigCallback<T>() {


Did somebody forget to write a test for this? 🤣

Yes. That was „I can do this quickly before going to bed“ 😓

Bombe · 2024-03-12T20:30:38Z

src/freenet/config/ConfigCallback.java

+	 *
+	 * @param set accepts the new value and return an Exception or null (if no exception)
+	 */
+	public static <T> ConfigCallback<T> from(Supplier<T> get, Function<T, Exception> set) {


I get why you’re doing this but functions returning exceptions rub me all kinds of wrong ways. I’d prefer something like this:

public interface ConfigConsumer<T> { void accept(T value) throws InvalidConfigValueException, NodeNeedRestartException; }

This way we don’t need casting in the set() method, and we don’t have to return anything from the Consumer.

that’s much nicer — thank you!

Bombe · 2024-03-12T20:33:38Z

src/freenet/node/NodeIPDetector.java

@@ -63,7 +65,7 @@ public void shouldUpdate(){
 	/** Explicit forced IP address in string form because we want to keep it even if it's invalid and therefore unused */
 	String overrideIPAddressString;
 	/** config: allow bindTo localhost? */
-	boolean allowBindToLocalhost;
+	Boolean allowBindToLocalhost;


Is this actually necessary? The compiler should take care of all the boxing/unboxing…

No it isn’t — thank you!

Bombe · 2024-03-12T20:38:37Z

src/freenet/support/api/BooleanCallback.java

 import freenet.config.ConfigCallback;

 /**
 * A callback to be called when a config value of integer type changes.
 * Also reports the current value.
 */
 public abstract class BooleanCallback extends ConfigCallback<Boolean> {
+  public static BooleanCallback fromBoolean(Supplier<Boolean> get, Function<Boolean, Exception> set) {
+    return (BooleanCallback) ConfigCallback.from(get, set);


Uhm… this is not how casting works. ConfigCallback.from returns a ConfigCallback object, no amount of casting will magically turn it into a BooleanCallback.

You know, you would have caught that if you had written tests for this. 😠

ArneBab · 2024-03-16T22:31:01Z

accidentally merged, then undid that merging

ArneBab · 2024-03-16T22:39:38Z

I didn’t find a way to actually undo the closing here, so please check here: #951

I’m sorry for the inconvenience :-(

ArneBab commented Sep 3, 2023

View reviewed changes

src/freenet/node/NodeIPPortDetector.java Outdated Show resolved Hide resolved

ArneBab force-pushed the allow-127.0.0.1-in-bindTo branch from aa8bc85 to f9b449a Compare September 9, 2023 03:21

ArneBab force-pushed the allow-127.0.0.1-in-bindTo branch from a168b0d to 86bca5f Compare February 18, 2024 09:08

ArneBab force-pushed the allow-127.0.0.1-in-bindTo branch from 86bca5f to f783938 Compare February 18, 2024 09:09

Bombe requested changes Feb 20, 2024

View reviewed changes

Bombe requested changes Mar 12, 2024

View reviewed changes

ArneBab closed this Mar 16, 2024

ArneBab force-pushed the allow-127.0.0.1-in-bindTo branch from ec8a54f to 94d4685 Compare March 16, 2024 22:30

ArneBab deleted the allow-127.0.0.1-in-bindTo branch March 16, 2024 22:32

ArneBab restored the allow-127.0.0.1-in-bindTo branch March 16, 2024 22:32

ArneBab mentioned this pull request Mar 16, 2024

Allow 127.0.0.1 in bind to #951

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow 127.0.0.1 in bindTo #851

allow 127.0.0.1 in bindTo #851

ArneBab commented Aug 29, 2023

ArneBab commented Feb 18, 2024

Bombe Feb 20, 2024

Bombe Feb 20, 2024

Bombe Feb 20, 2024

gits7r commented Feb 21, 2024 •

edited

Loading

Bombe Mar 12, 2024

ArneBab Mar 16, 2024

Bombe Mar 12, 2024

ArneBab Mar 16, 2024

Bombe Mar 12, 2024

ArneBab Mar 16, 2024

Bombe Mar 12, 2024

ArneBab commented Mar 16, 2024

ArneBab commented Mar 16, 2024 •

edited

Loading

allow 127.0.0.1 in bindTo #851

allow 127.0.0.1 in bindTo #851

Conversation

ArneBab commented Aug 29, 2023

ArneBab commented Feb 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gits7r commented Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArneBab commented Mar 16, 2024

ArneBab commented Mar 16, 2024 • edited Loading

gits7r commented Feb 21, 2024 •

edited

Loading

ArneBab commented Mar 16, 2024 •

edited

Loading